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

Remove SingletonManager and related code #5282

Open
wants to merge 1 commit into
base: 3.1
Choose a base branch
from

Conversation

ctubbsii
Copy link
Member

Remove last remaining static singleton by attaching tablet locator code to the ClientContext

Remove last remaining static singleton by attaching tablet locator code
to the ClientContext
@ctubbsii ctubbsii added this to the 3.1.0 milestone Jan 24, 2025
@ctubbsii
Copy link
Member Author

There's still some places that look sketchy... like TabletLocator objects leaking outside try-catch blocks where the ClientContext lives.

Copy link
Contributor

@keith-turner keith-turner left a comment

Choose a reason for hiding this comment

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

There's still some places that look sketchy... like TabletLocator objects leaking outside try-catch blocks where the ClientContext lives.

The tablet locator objects could be closed when the client context is closed, causing them to throw an exception if used. That could be follow on work.

+ "disabled. This is likely caused by all AccumuloClients being closed or garbage collected");
LocatorKey key = new LocatorKey(context.getInstanceID(), tableId);
TabletLocator tl = locators.get(key);
final var locators = context.tabletLocators();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could eventually transition away from this static synchronization and use concurrent hash map for the table map and sync in its compute function. Could also remove these static methods and transition this to the client context which internalyl uses a concurrent hash map for sync.

Wonder if the code in the method were replaced w/ a single call to client context if it would be easy to inline this method.

@@ -340,7 +340,7 @@ private void logBusyTablets(List<ComparablePair<Long,KeyExtent>> busyTablets,
this.security = context.getSecurityOperation();

watchCriticalScheduledTask(context.getScheduledExecutor().scheduleWithFixedDelay(
TabletLocator::clearLocators, jitter(), jitter(), TimeUnit.MILLISECONDS));
() -> TabletLocator.clearLocators(context), jitter(), jitter(), TimeUnit.MILLISECONDS));
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be able to be removed, needs investigation. I think the reason this code is here is that bulk v1 code would populate user tablet info in the locator cache. This would periodically clear that in case user tables were deleted to avoid memory build up. Now the tablet server should only have accumulo tables in the tablet locator cache.

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

Successfully merging this pull request may close these issues.

2 participants