Re: [MirageOS-devel] Irmin API evolution

[Thomas Leonard <talex5@xxxxxxxxx>]

On 7 September 2015 at 10:56, Thomas Gazagnaire <thomas@xxxxxxxxxxxxxx> wrote:
>> However, I think we should remove "create" from the store interfaces
>> anyway. In Irmin-IndexedDB, I want to pass database connections to the
>> internal RW and AO stores, not a config. It looks like I can attach
>> arbitrary data to config objects, but doing this loses compile-time
>> type checking (it would be possible to call it without providing a
>> connection).
>
> I agree. Your examples are convincing, and I think having the high-level and 
> low-level stores having a different interface makes sense as it will allow to 
> make the best design choices at each layer.
>
>>>> [ And ideally, it would make more sense to me if you only specified
>>>> the commit message when making a commit. The rest of the strings just
>>>> get thrown away, I think. ]
>>>
>>> The initial idea was to use that task to (i) populate an audit log on all 
>>> the database operations (including reads) and (ii) attach the debug 
>>> messages to the task, instead of throwing them on the error channel. None 
>>> of these have been completed yet, but would be nice if they are still 
>>> possible to do later.
>>
>> These things don't sound very Irmin-specific, or connected to the Git
>> log message. Perhaps we could provide a wrapper for logging?
>>
>> module AuditedStore (S : BC.STORE) : sig
>>  include BC.STORE
>>  val create : Logger.t -> S.t -> t
>>  ...
>> end
>
> What if you want to commit on every read (costly, but it'd be possible to do 
> now by passing a flag in the config)?

Then AuditedStore.create can take a ('a -> Irmin.task) and require the
user to instantiate it before each operation. i.e. the same thing we
do now, but only for users of AuditedStore.

It does mean that you can't take some existing code and immediately
turn on auditing because (in the current Irmin design) audit messages
are supplied by the thing being audited (which is a bit odd). I think
a more reasonable design would be to supply a tag to
AuditedStore.create and have it attach that tag to auto-generated
commit messages.

e.g.

let store = AuditedStore.create ~tag:"httpd" underlying in
AuditedStore.read store ["foo"] ...

would generate a commit entry in the form

2015-10-03 10:15: [httpd] read "/foo"

But I think the current system of requiring every user of Irmin to
supply a custom audit message for each operation, just in case, isn't
a good idea.

> Also, my initial idea for logging was to pass a store handle (and its task) 
> to the Log.debug functions, so that the log line is appended in the task's 
> commit message (that's why there is a `Task.add`). By doing so, we could have 
> all the debug message for a specific high-level command appears in the commit 
> message. Not sure how practical is it, but I though it could be a nice 
> feature.

You could have:

AuditedStore.logf store "Some log message"

create a commit with the given message (or save it for the next commit).

Actually, I was thinking of removing BC.update completely and
requiring everything to go via a transaction. That way, log messages
are supplied only to View.create and we can remove tasks from the rest
of the API. In that case, it would make sense to have a View.logf,
such that committing the transaction includes all the log messages
collected during it.

> I'm not sure how you can do that if you remove the task from all the read 
> operations.
>
> On the other hand, having immutable, simple read-operations is very useful 
> when designing the API. Currently, the HTTP "REST" [1] API doesn't require a 
> client task for read operations. Would be good to have a consistent API 
> across all backends, so that could be an argument for dropping the task for 
> high-level reads.
>
>
> About the wrapper: it's a good idea, we could have a default "audit" branch 
> to store this log.
>
> Thomas
>
> [1] https://github.com/mirage/irmin/issues/282#issuecomment-136806513



-- 
Dr Thomas Leonard        http://roscidus.com/blog/
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/mirageos-devel