-
Notifications
You must be signed in to change notification settings - Fork 628
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
ISPN-5528 Expose segment ownership in the Hot Rod client #3532
Conversation
rebased |
This PR should be rebased on top of #3511, marking as preview for now |
50437c4
to
9f8a18f
Compare
Rebased |
Failures not caused by this PR: Looked at the agents and all have free disk space and /tmp is apparently writable, need to investigate the cause... |
rebased |
/** | ||
* Returns segment ownership for this cache | ||
*/ | ||
Map<SocketAddress, Set<Integer>> getSegmentsByServer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I like this exposed as public API. Can't we just leave it in RemoteCacheImpl and have code that needs to use this cast to the actual implementation ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your concern of exposing this as public API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is definitely not a user-oriented API: however, once it ends up in the public API we have to keep it stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is definitely not a user-oriented API
Yeah, it's like a SPI, maybe an advanced API 😄
One of the Remotecache.retrieveEntries
accepts a list of segments to filter, IMO it'd be a bit odd to hide getSegmentsByServer
since it's a useful complement. We discussed previously the possibility of a AdvancedRemoteCache
interface, it's not too late to rescue that idea if there's strong opposition to expose segments & co in the RemoteCache
we have to keep it stable
Not trying to predict the future, but segment ownership by servers is a solidified concept that is not likely to go away anytime soon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I understand, but then I'd probably prefer to encapsulate the return type into an interface like RemoteSegmentMapping instead of Map<SocketAddress, Set> (same for retrieveEntries).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to encapsulate the naked Map
Updated with changes to the API |
Looks good, integrated :) |
https://issues.jboss.org/browse/ISPN-5528
Changes in the server:
numSegments
for clustered caches.Changes in the client:
getSegmentsByServer()
method that:{0...numSegments-1}
numSegments
andhashFunction
are present (DIST) uses the consistent hash to associate the segments to each server