HDDS-4876. [SCM HA Security] Add failover proxy to SCM Security Server Protocol#1978
Conversation
bharatviswa504
force-pushed
the
HDDS-4876
branch
2 times, most recently
from
623fabb to
83d907f
Compare
There was a problem hiding this comment.
@bharatviswa504 , what will happen if SCM HA mode is disabled and no serviced id or node ids are defined.?
There was a problem hiding this comment.
buildNodeInfo() takes care of HA/non-HA config, and returns scmNodeInfo.
There was a problem hiding this comment.
When serviceId and nodeId are not defined, we use some const values and make the proxy failover work for HA/non-HA.
bharatviswa504
force-pushed
the
HDDS-4876
branch
from
efa6b26 to
dc6823c
Compare
bshashikant
left a comment
bshashikant
left a comment
There was a problem hiding this comment.
Looks good with minor change.
xiaoyuyao
left a comment
xiaoyuyao
left a comment
There was a problem hiding this comment.
Thanks @bharatviswa504 for working on this. The changes LGTM overall, a few comments added inline.
There was a problem hiding this comment.
NIT: can you change message to cause and the type to Throwable?
There was a problem hiding this comment.
Do we have a separate configuration for serviceId? If yes, we should assert the one from nodeinfo match with the smcServiceId from config instead of rewriting it each time a new node is loaded.
There was a problem hiding this comment.
Yes, we have a separate config for service id.
BuildNodeInfo got the list of ScmNodeInfo for a single scm serviceId, so all scmNodeInfo will have same scmServiceID.
There was a problem hiding this comment.
Do we handle failover with leader hints in addition to RR failover?
There was a problem hiding this comment.
Currently in SCM we don't have leader hints, Added a TODO for this.
There was a problem hiding this comment.
Synchronized on the method for thread safe proxy creation.
There was a problem hiding this comment.
can we go through the proxy map, call stopProxy and null the value in the map?
There was a problem hiding this comment.
The same util exists in OmUtils, can we dedup the code with one copy?
There was a problem hiding this comment.
It has additional InvalidToken, for that OM has special logic, where it retries all 3 OMs atleast once. So, added a new method.
When SCM also has similar logic for TokenInvalid we can have a single copy of this method.
There was a problem hiding this comment.
Opened a new Jira for this
https://issues.apache.org/jira/browse/HDDS-4934
There was a problem hiding this comment.
should we switch the order of 129-130, and 123-128 so that the exception message from the cause can be preserved in scmSecurityResponse.message?
bharatviswa504
force-pushed
the
HDDS-4876
branch
from
2603914 to
54dec6a
Compare
|
Thank You @xiaoyuyao for the review addressed/replied to review comments. |
|
Thanks @bharatviswa504 for the update. LGTM, +1. |
|
Thank You @xiaoyuyao and @bshashikant for the review. |
3 participants
What changes were proposed in this pull request?
Implement failover proxy provider for SCM Security Server Protocol.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4876
How was this patch tested?
Updated few tests and also existing tests should cover. (For HA, later tests will be added end to end)