Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RANGER-5131: remove finalize from policy engine #531

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

RANGER-5131: remove finalize from policy engine #531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7be3ec
removed the finalize method from policy engine
Feb 11, 2025
c72b2bb
Merge branch 'apache:master' into RANGER-5131
vyommani Feb 12, 2025
2210038
removed the cleanup methods as precleanup seems to be doing all work
Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions ...src/main/java/org/apache/ranger/plugin/contextenricher/RangerAbstractContextEnricher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,13 +300,4 @@ public Properties readProperties(String fileName) {

return ret;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay to simply remove finalize() methods, without an alternate method to execute the cleanup performed currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the finalize() method without an alternative cleanup method is not advisable. However, my analysis reveals that we already have robust cleanup mechanisms in place.
Upon reviewing the Ranger source code, I found that the PolicyEngine is referenced in the following classes:

RangerPolicyAdminImpl
RangerPolicyEngineImpl

Both RangerPolicyAdminImpl and RangerPolicyEngineImpl have a releaseResources(boolean isForced) method, which calls the preCleanup(isForced) method on the PolicyEngine. This preCleanup method performs the following cleanup operations:

Calls policyRepository.preCleanup()
Calls tagPolicyRepository.preCleanup()
Calls zonePolicyRepositories.preCleanup()

These preCleanup methods on the repository iterate through the Context Enrichers and call preCleanup on individual instances.

Upon further review, I noticed that the setPolicies method in RangerBasePlugin calls the releaseResources method on RangerPolicyEngineImpl. This ensures that resources held by the old policy engine are properly released before updating to the new policy.

Given that we already have alternative cleanup method (releaseResources), I believe we can safely remove the finalize() method. Please review my findings and let me know if I've missed any code paths.

@Override
protected void finalize() throws Throwable {
try {
cleanup();
} finally {
super.finalize();
}
}
}
39 changes: 0 additions & 39 deletions agents-common/src/main/java/org/apache/ranger/plugin/policyengine/PolicyEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,15 +338,6 @@ public String toString() {
return toString(new StringBuilder()).toString();
}

@Override
protected void finalize() throws Throwable {
try {
cleanup();
} finally {
super.finalize();
}
}

public StringBuilder toString(StringBuilder sb) {
if (sb == null) {
sb = new StringBuilder();
Expand Down Expand Up @@ -606,36 +597,6 @@ private void reorderPolicyEvaluators() {
LOG.debug("<== reorderEvaluators()");
}

private void cleanup() {
LOG.debug("==> PolicyEngine.cleanup()");

RangerPerfTracer perf = null;

if (RangerPerfTracer.isPerfTraceEnabled(PERF_POLICYENGINE_INIT_LOG)) {
perf = RangerPerfTracer.getPerfTracer(PERF_POLICYENGINE_INIT_LOG, "RangerPolicyEngine.cleanUp(hashCode=" + Integer.toHexString(System.identityHashCode(this)) + ")");
}

preCleanup(false);

if (policyRepository != null) {
policyRepository.cleanup();
}

if (tagPolicyRepository != null) {
tagPolicyRepository.cleanup();
}

if (MapUtils.isNotEmpty(this.zonePolicyRepositories)) {
for (Map.Entry<String, RangerPolicyRepository> entry : this.zonePolicyRepositories.entrySet()) {
entry.getValue().cleanup();
}
}

RangerPerfTracer.log(perf);

LOG.debug("<== PolicyEngine.cleanup()");
}

private void getDeltasSortedByZones(PolicyEngine current, ServicePolicies servicePolicies, List<RangerPolicyDelta> defaultZoneDeltas, List<RangerPolicyDelta> defaultZoneDeltasForTagPolicies) {
LOG.debug("==> getDeltasSortedByZones()");

Expand Down
23 changes: 0 additions & 23 deletions ...ts-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,15 +313,6 @@ public String toString() {
return sb.toString();
}

@Override
protected void finalize() throws Throwable {
try {
cleanup();
} finally {
super.finalize();
}
}

public StringBuilder toString(StringBuilder sb) {
if (sb == null) {
sb = new StringBuilder();
Expand Down Expand Up @@ -482,20 +473,6 @@ void preCleanup(boolean isForced) {
LOG.debug("<== preCleanup(isForced={})", isForced);
}

void cleanup() {
LOG.debug("==> cleanup()");

preCleanup(false);

if (CollectionUtils.isNotEmpty(this.contextEnrichers) && !isContextEnrichersShared) {
for (RangerContextEnricher enricher : this.contextEnrichers) {
enricher.cleanup();
}
}

LOG.debug("<== cleanup()");
}

void reorderPolicyEvaluators() {
LOG.debug("==> reorderEvaluators()");

Expand Down