geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anthony Baker <aba...@apache.org>
Subject Re: Review Request 57466: GEODE-2632: check if it is integrated security early in the call
Date Fri, 10 Mar 2017 00:15:52 GMT


> On March 9, 2017, 6:27 p.m., Kirk Lund wrote:
> > Here's the change I recommend for improving performance. The problem is this line...
> > 
> > private static SecurityService defaultInstance = new IntegratedSecurityService();
> > 
> > It always installs a IntegratedSecurityService. Branch here and install IntegratedSecurityService
iif we have security enabled; else we install NullSecurityService which is a no-op that does
nothing but return false, etc.
> 
> Jinmei Liao wrote:
>     but we need the object to determine whether it is enabled or not. With it being null,
how can we determine if we have security enabled or not?
> 
> Kirk Lund wrote:
>     It's not null, the name is NullSecurityService because it follows the Null Object
Pattern. Imagine if you call it NoOpSecurityService or NoSecurityService.
> 
> Kirk Lund wrote:
>     And it requires further refactoring. The static "getSecurityService" would need to
move to a different class.
>     
>     SecurityService securityService = SecurityService.getSecurityService();
>     
>     If Security is enabled then the above returns an instance of IntegratedSecurityService.
>     
>     If Security is disabled then the above returns an instance of NullSecurityService.
All of the methods return false, null or if return type is void then the method is simply
empty. JIT will eventually remove the calls to methods that are empty so runtime gets even
faster.
> 
> Jinmei Liao wrote:
>     Oh, I see what you mean. As it requires more refactoring (pulling the code to initSecurity,
and check for isIntegratedSecurity out of the interface), can we do this for now, and leave
the ticket open for this further improvement?
> 
> Kirk Lund wrote:
>     Definitely. We'll probably want to do this iteratively. We should look into writing
a new JMH benchmark to help with this process.

+1 for JMH testing.  We need to drive performance changes from data and JMH is great for assessing
performance through micro-benchmarks.  Concretely, I'd like to see this called out in the
commit comment, e.g. "Implement null object pattern for security to optimize performance.
 Results in an improvement of up to XXX% when security is disabled."


- Anthony


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57466/#review168485
-----------------------------------------------------------


On March 9, 2017, 3:52 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57466/
> -----------------------------------------------------------
> 
> (Updated March 9, 2017, 3:52 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, and Kirk Lund.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> * clean up interface, reduce function call
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java
8366930a3bf6a244d077c745fc810d77c4699c38 
>   geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java 14784c391212095413c0d577cfc65de7247080b5

>   geode-core/src/main/java/org/apache/geode/management/internal/security/AccessControlMBean.java
6514a33e52611994ddc16a58414146ebaad75c65 
>   geode-core/src/main/java/org/apache/geode/security/ResourcePermission.java 45da464419779773c9116d824fcf11774bafbd79

> 
> 
> Diff: https://reviews.apache.org/r/57466/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin successful
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message