Conversation
Renamed the thrift class from ManagerClientService to PrimaryManagerClientService
Sets up an AssistantManagerThriftClient to use instead of the PrimaryManager.
Use the AssistantManager Client instead of the PrimaryManager Client for flush actions
Moves table, system, and namespace property actions to the assistantManagers
Moves the resourceGroup property actions from PrimaryManager to AssistantManager
keith-turner
left a comment
keith-turner
left a comment
There was a problem hiding this comment.
For testing I suspect ComprehensiveMultiManagerIT will cover a lot of these changes for the multiple manager case. Would be good to review the APIs it calls and the APIs changed here to be sure.
|
|
||
| @Override | ||
| public Client getConnection(ClientContext context) { | ||
| return getManagerConnection(LOG, this, context); |
There was a problem hiding this comment.
Seems like this will always connect to the primary manager. Need to call [this code] (
For picking one I have been considering using hashing for #6279 to better spread the connections. If we randomly pick one and we C clients then every assistant manager can have up to C connections or more. If we hash clients to AM assistant managers then each one will have on the order of C/AM connections. This could also help w/ debugging where a client goes to the same manager consistently. However not sure what to hash on in the client. For #6279 was thinking of doing hash mod using the compactor address. To start with it would probably be best to just randomly pick one though.
There was a problem hiding this comment.
It would be nice if we could tie in TServerClient.getThriftServerConnection somehow. Then the debug property would apply to Manager connections as well.
There was a problem hiding this comment.
@keith-turner I saw the code in FateManager, but the FateThriftClient still used the default getConnection method from ManagerClient.
Should FateThriftClient also be updated to select from the assistant Managers?
There was a problem hiding this comment.
Should FateThriftClient also be updated to select from the assistant Managers?
Can not do that now, fate meta client is only available on the primary manager. After #6224,#6227, and #6232 are merged then both fate clients (meta and user) would be available on all managers. After those are merged could refactor the client code that initiates fate table ops to use any manger.
@keith-turner I saw the code in FateManager, but the FateThriftClient still used the default getConnection method from ManagerClient.
The code in FateThriftClient is for clients that interact with creating and waiting on fate operations. The code in FateManager is distributing execution of fate operations across managers. So what we currently have is fate operations can execute on all managers, but accumulo clients must still work w/ the primary manager to initiate and wait on fate operations. After #6232 is merged then accumulo clients could interact w/ any manager.
There was a problem hiding this comment.
okay! I opened #6291 for simplifying the thrift client code a bit.
| return getManagerConnection(LOG, this, context); | ||
| } | ||
|
|
||
| public <R> R executeTableCommand(ClientContext context, Exec<R,Client> exec) |
There was a problem hiding this comment.
Was this code copied from MangerThriftClient? If so would be good to push to the super type and refactor for any specialization needed.
There was a problem hiding this comment.
May need to create a new super type that the primary and assistant manager share, not sure. See that a fate client also extends the base type and it may not need some of these methods.
| import org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException; | ||
| import org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException; | ||
| import org.apache.accumulo.core.manager.thrift.ManagerClientService.Client; | ||
| import org.apache.accumulo.core.manager.thrift.PrimaryManagerClientService.Client; |
There was a problem hiding this comment.
would be good to rename this class from ManagerThriftClient to PrimaryManagerThriftClient
| try { | ||
| // Acquire the lock that all managers get before the primary lock, this allows non primary | ||
| // manager processes to work on stuff. | ||
| getAssistantManagerLock(); |
There was a problem hiding this comment.
getAssistantManagerLock() should also change to advertise the new services the AssistantManager is servicing. This would allow clients to get the addresses for all managers, then select one that is advertising the service that the client needs. TServerClient.getThriftServerConnection does this right now, but only for a subset of services. The logic in that class could be expanded to handle more types.
3 participants
Renames the current ManagerClientService to PrimaryManagerClientService.
Adds an AssistantManagerClientService thrift RPC to the manager.
Migrated flush and property actions to the AssistantManagerClientService.
relates to #6183