jackrabbit-oak-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chetan Mehrotra (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (OAK-3146) ExternalLoginModuleFactory should inject SyncManager and ExternalIdentityProviderManager
Date Tue, 28 Jul 2015 09:02:04 GMT

    [ https://issues.apache.org/jira/browse/OAK-3146?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14644046#comment-14644046
] 

Chetan Mehrotra edited comment on OAK-3146 at 7/28/15 9:01 AM:
---------------------------------------------------------------

bq.  you filed this issue with the label 'performance' but fixed the issue without even verifying
i.e. writing a benchmark test that would prove that you actually found/fixed a performance
issue.

For me the issue was logical one as its knows that frequent lookup on ServiceRegistry is slow
and becomes a bottleneck. Reproducing such performance issue is tricky and hence I went ahead
with the proposed fix based on _logical_ understanding of the problem and from my previous
experience in such issues

bq.  in case of a performance issue this should be covered with benchmarks, such that we reproduce
the issue first and then fix it and in addition verify later on, that the fix doesn't get
broken later on

Ack that benchmark is always better. 

However for performance issues many times the fix is done based on logical flow and following
best practices. For e.g. if you have a code which makes same remote call in a loop. Then it
can easily be fixed by moving the remote call out of the loop and reuse the response. This
would be considered by me as a logical fix and having a benchmark in such cases is not _must_.


In similar way here the code was doing a service lookup on each initialize call which with
proposed fix was done once. There was a backing testcase to further validate that new approach
is being used. Based on that I did not worked upon adding a benchmark. If thats not acceptable
to you then thats also fine with me. Anyways the changes have been reverted and patch is attached
for someone to folowup later

bq. there is not auto-backport for improvements and i want you to revert that.

Have reverted the various changes now

Unassigning the issue now as I would not have time to create a benchmark for this issue


was (Author: chetanm):
bq.  you filed this issue with the label 'performance' but fixed the issue without even verifying
i.e. writing a benchmark test that would prove that you actually found/fixed a performance
issue.

For me the issue was logical one as its knows that frequent lookup on ServiceRegistry is slow
and becomes a bottleneck. Reproducing such performance issue is tricky and hence I went ahead
with the proposed fix based on _logical_ understanding of the problem and from my previous
experience in such issues

bq. there is not auto-backport for improvements and i want you to revert that.

Have reverted the various changes now

Unassigning the issue now as I would not have time to create a benchmark for this issue

> ExternalLoginModuleFactory should inject SyncManager and ExternalIdentityProviderManager
> ----------------------------------------------------------------------------------------
>
>                 Key: OAK-3146
>                 URL: https://issues.apache.org/jira/browse/OAK-3146
>             Project: Jackrabbit Oak
>          Issue Type: Improvement
>          Components: security
>            Reporter: Chetan Mehrotra
>            Priority: Minor
>              Labels: performance
>             Fix For: 1.3.4
>
>         Attachments: OAK-3146.patch
>
>
> {{ExternalLoginModule}} currently performs a lookup of {{SyncManager}} and {{ExternalIdentityProviderManager}}
in initialize call which performs service lookup using ServiceTracker. Doing a service lookup
in critical path would have adverse performance impact. 
> Instead of performing the lookup {{ExternalLoginModuleFactory}} should track the two
manager instances and pass them to {{ExternalLoginModule}}. This would ensure that expensive
operation like service lookup is not performed in critical path



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message