Add the #setHook method to the NodeStore?

[Alex Parvulescu <al...@gmail.com>]

hi guys,

I'm wondering if we could promote the #setHook method to the NodeStore api?

CommitHooks are an important part of Oak and all the existing
implementations have one anyway.

thanks,
alex

Re: Add the #setHook method to the NodeStore?

[Angela Schreiber <an...@adobe.com>]

see https://issues.apache.org/jira/browse/OAK-627

On 2/14/13 6:38 PM, Angela Schreiber wrote:
> hi
>
>>> I'm wondering if we could promote the #setHook method to the NodeStore api?
>>
>> The intention so far has been to keep NodeStore functionally
>> equivalent to the MicroKernel, i.e. just a higher-level wrapper that
>> hides details like caching, JSOP processing and revision/path
>> tracking. Adding commit hooks would notably change that. Ideally I'd
>> see RootImpl instead of the underlying NodeStore applying the commit
>> hooks.
>
> that would also simplify things for having separate commit hook
> instances depending on the 'nature' of the root... for example
> creating hooks that know about the modified workspace (see OAK-625,
> which currently seems to get a bit hacky) or skip commit hooks
> in specific scenarios....
>
>> The implementation dependency to KernelNodeStore.setHook() is a
>> hacky solution as you noticed.
>
> doesn't matter as long we are free to clean it up once we have
> a better solution.
>
>> So until we figure out how to move hooks away from the NodeStore
>> implementation, it probably does make sense to reflect that in the
>> interface. However, instead of a setHook() method, I'd rather do this
>> with an additional CommitHook argument to the merge() command, as
>> that'll make it easier to later move the hook processing to RootImpl.
>
> i just had a quick look how we could get there but i don't feel
> comfortable enough with the merge/commit get it done quickly.
> anybody? or shall i create an issue for that?
>
> regards
> angela
>
>> BR,
>>
>> Jukka Zitting

Re: Add the #setHook method to the NodeStore?

[Angela Schreiber <an...@adobe.com>]

hi jukka

now that you have committed an initial patch for OAK-627 (thanks)
i will check if there wasn't a smarter solution for OAK-625...
update will follow in the issue.

gruss
angela

On 2/15/13 1:48 PM, Jukka Zitting wrote:
> Hi,
>
> On Thu, Feb 14, 2013 at 7:38 PM, Angela Schreiber<an...@adobe.com>  wrote:
>> that would also simplify things for having separate commit hook
>> instances depending on the 'nature' of the root... for example
>> creating hooks that know about the modified workspace (see OAK-625,
>> which currently seems to get a bit hacky) or skip commit hooks
>> in specific scenarios....
>
> Good point!
>
> BR,
>
> Jukka Zitting

Re: Add the #setHook method to the NodeStore?

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Thu, Feb 14, 2013 at 7:38 PM, Angela Schreiber <an...@adobe.com> wrote:
> that would also simplify things for having separate commit hook
> instances depending on the 'nature' of the root... for example
> creating hooks that know about the modified workspace (see OAK-625,
> which currently seems to get a bit hacky) or skip commit hooks
> in specific scenarios....

Good point!

BR,

Jukka Zitting

Re: Add the #setHook method to the NodeStore?

[Angela Schreiber <an...@adobe.com>]

hi

>> I'm wondering if we could promote the #setHook method to the NodeStore api?
>
> The intention so far has been to keep NodeStore functionally
> equivalent to the MicroKernel, i.e. just a higher-level wrapper that
> hides details like caching, JSOP processing and revision/path
> tracking. Adding commit hooks would notably change that. Ideally I'd
> see RootImpl instead of the underlying NodeStore applying the commit
> hooks.

that would also simplify things for having separate commit hook
instances depending on the 'nature' of the root... for example
creating hooks that know about the modified workspace (see OAK-625,
which currently seems to get a bit hacky) or skip commit hooks
in specific scenarios....

> The implementation dependency to KernelNodeStore.setHook() is a
> hacky solution as you noticed.

doesn't matter as long we are free to clean it up once we have
a better solution.

> So until we figure out how to move hooks away from the NodeStore
> implementation, it probably does make sense to reflect that in the
> interface. However, instead of a setHook() method, I'd rather do this
> with an additional CommitHook argument to the merge() command, as
> that'll make it easier to later move the hook processing to RootImpl.

i just had a quick look how we could get there but i don't feel
comfortable enough with the merge/commit get it done quickly.
anybody? or shall i create an issue for that?

regards
angela

> BR,
>
> Jukka Zitting

Re: Add the #setHook method to the NodeStore?

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Thu, Feb 14, 2013 at 1:13 PM, Alex Parvulescu
<al...@gmail.com> wrote:
> I'm wondering if we could promote the #setHook method to the NodeStore api?

The intention so far has been to keep NodeStore functionally
equivalent to the MicroKernel, i.e. just a higher-level wrapper that
hides details like caching, JSOP processing and revision/path
tracking. Adding commit hooks would notably change that. Ideally I'd
see RootImpl instead of the underlying NodeStore applying the commit
hooks. The implementation dependency to KernelNodeStore.setHook() is a
hacky solution as you noticed.

So until we figure out how to move hooks away from the NodeStore
implementation, it probably does make sense to reflect that in the
interface. However, instead of a setHook() method, I'd rather do this
with an additional CommitHook argument to the merge() command, as
that'll make it easier to later move the hook processing to RootImpl.

BR,

Jukka Zitting