vyommani commented on code in PR #595:
URL: https://github.com/apache/ranger/pull/595#discussion_r2306386136
##########
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java:
##########
@@ -583,6 +584,9 @@ public void setPolicies(ServicePolicies policies) {
setServiceConfigs(policies.getServiceConfig());
+ this.synchronousPolicyRefresh =
isSynchronousPolicyRefresh();
Review Comment:
Thank you for sharing the proposed change to reduce test wait times for
policy/tag synchronization in RangerBasePlugin. I understand the goal is to
minimize delays by introducing a configuration to control policy refreshes.
However, I have concerns about the approach of calling refreshPoliciesAndTags()
within isAccessAllowed() when the synchronousPolicyRefresh flag is enabled.
isAccessAllowed() is a public API designed solely to evaluate access
permissions. Embedding a call to refreshPoliciesAndTags(), even if guarded by a
flag, introduces a secondary responsibility of policy/tag synchronisation. This
violates SRP and makes the method’s behaviour less predictable, as callers may
not expect a refresh to occur.
Both isAccessAllowed() and refreshPoliciesAndTags() are public methods with
distinct purposes. Mixing their responsibilities, even under a configuration
flag, could lead to unintended side effects. Since isAccessAllowed() is a
performance-critical API, enabling synchronous refreshes could significantly
slow down production systems if the flag is misconfigured by a client. This
risk is particularly concerning for a public API where users expect consistent
and predictable behaviour.
I’m curious did you explore calling refreshPoliciesAndTags() explicitly
before invoking isAccessAllowed(). While modifying every test to include an
explicit
refresh may not be ideal, other options could be considered, such as, Adjust
the default sync interval (30s to 200ms or 1 second) to minimize wait times
without
embedding refresh logic in isAccessAllowed().
Have you conducted performance tests to evaluate the impact of this change?
Even if the additional if condition for the flag adds minimal overhead (a few
nanoseconds ), understanding the performance implications is critical,
particularly for a public API used in production environments.
I recommend keeping isAccessAllowed() focused on its core responsibility and
exploring alternative ways to achieve the goal of reducing test wait times.
Let’s maintain the separation of concerns between these public APIs to ensure
clarity, predictability, and performance.
Please let me know your thoughts or if there are additional considerations
(e.g., specific constraints that led to this design). I’d be happy to discuss
further or assist in exploring other solutions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@ranger.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
