-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-25383: Ability to update and remove peer base config #2778
Conversation
🎊 +1 overall
This message was automatically generated. |
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.
(non-binding) - Seems like there are some edge cases to this approach that still need to be worked through.
...ient/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
Outdated
Show resolved
Hide resolved
...ient/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
...ient/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
Outdated
Show resolved
Hide resolved
...ient/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
Outdated
Show resolved
Hide resolved
8916bec
to
72d293a
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
A bunch of nits but lgtm. I think this approach is easy to reason about compared to the original version of the patch. There will be some weird edge cases here too but I think we can live with them.
@gjacoby126 any thoughts on this approach or is this good to go?
...ient/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
Show resolved
Hide resolved
getConfiguration().get(secondCustomPeerConfigKey)); | ||
} finally { | ||
shutDownMiniClusters(); | ||
baseConfiguration.unset(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG); | ||
} | ||
} | ||
|
||
@Test | ||
public void testBasePeerConfigsRemovalForPeerMutations() |
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.
nit: Use a word other than mutation given it has a different meaning in HBase context? or may be just call it testBasePeerConfigRemoval..
} | ||
|
||
@Test | ||
public void testRemoveBasePeerConfigWithoutExistingConfigForPeerMutations() |
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.
same nit as above.
I think this approach of removing key if
For rolling upgrade, updating configs in new bits is common before performing rolling restart of master and RS (which can also update config symlinks e.g /etc/conf/hbase), hence this patch should fit well for upgrades IMHO. With this patch, it's just like updating any other config. +1 for the PR overall once @bharathv and @gjacoby126 's remaining comments are addressed. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
+1 (non-binding) after fixing the one remaining nit below
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
+1
Closes #2778 Signed-off-by: Bharath Vissapragada <bharathv@apache.org> Signed-off-by: Geoffrey Jacoby <gjacoby@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Closes #2778 Signed-off-by: Bharath Vissapragada <bharathv@apache.org> Signed-off-by: Geoffrey Jacoby <gjacoby@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Closes #2778 Signed-off-by: Bharath Vissapragada <bharathv@apache.org> Signed-off-by: Geoffrey Jacoby <gjacoby@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Currently, we cannot update the peer based config even if we change the value of config.
Secondly, once the configuration is added, there is not smooth way to remove the peer config.
This patch adds an ability to update and remove the peer config if required.