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.

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

HDDS-7178. [Multi-Tenant] Use optimistic read in Ranger background sync #3725

Merged
merged 5 commits into from Feb 17, 2023

Conversation

errose28
Copy link
Contributor

What changes were proposed in this pull request?

Coordination between the background sync and tenant modification requests is currently done through a stamped lock. The background sync currently holds a read lock for a short period every time it runs to fetch a consistent view of OM and Ranger state, however, tenant modifications will be blocked during this time since they need the write lock.

Since this is already a stamped lock, we can use optimistic read with retries instead of a read lock in the background sync. This way it will not conflict with user tenant modifications as long as there is no divergence between OM and Ranger. If there is divergence, the background sync will still need to push updates under a write lock, blocking tenant modifications while it does so.

This PR also adds some logging improvements to the background sync.

What is the link to the Apache JIRA

HDDS-7178

How was this patch tested?

  • Unit test added
  • Test background sync with live requests and a real Ranger instance (TODO)

@errose28
Copy link
Contributor Author

@smengcl @prashantpogde PTAL when you get a chance for feedback on this approach. This is pending testing with live Ranger as well. If we want to change the default frequency with which the background sync runs (it's currently every 10 minutes) we can do that in this PR too.

// state without a write operation interrupting.
int attempt = 0;
boolean readSuccess = false;
while (!readSuccess && attempt < MAX_ATTEMPT) {
Copy link
Contributor

@smengcl smengcl Aug 30, 2022

Choose a reason for hiding this comment

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

Let's use a different MAX_ATTEMPT constant to differentiate. Set the new one to maybe 3 or more.

Copy link
Contributor

@prashantpogde prashantpogde left a comment

Choose a reason for hiding this comment

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

This looks like a better approach. LGTM

@smengcl smengcl marked this pull request as ready for review September 7, 2022 18:13
Copy link
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

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

lgtm. Let's test this manually and merge this.


final AuthorizerLock authorizerLock = new AuthorizerLockImpl();

Assert.assertFalse(authorizerLock.isWriteLockHeldByCurrentThread());

// Read lock does not affect the check
long readLockStamp = authorizerLock.tryReadLockThrowOnTimeout();
long readLockStamp = authorizerLock.tryReadLock(100);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
long readLockStamp = authorizerLock.tryReadLock(100);
long readLockStamp = authorizerLock.tryReadLock(100L);


// With only competing reads, an optimistic read should be valid.
long optStamp = authorizerLock.tryOptimisticReadThrowOnTimeout();
long readStamp = authorizerLock.tryReadLock(100);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
long readStamp = authorizerLock.tryReadLock(100);
long readStamp = authorizerLock.tryReadLock(100L);

@smengcl
Copy link
Contributor

smengcl commented Oct 12, 2022

@errose28 Would you resolve the conflict and address the comments?

@DaveTeng0
Copy link
Contributor

hey @errose28 ~ the CI build step of findbugs failed~ please take a look when you have some time. thanks!

@kerneltime
Copy link
Contributor

@errose28 can you please rebase?

@adoroszlai adoroszlai merged commit 43caa56 into apache:master Feb 17, 2023
@adoroszlai
Copy link
Contributor

Thanks @errose28 for the improvement, @prashantpogde, @smengcl for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants