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-24759 Refuse to update configuration of default group #2126
Conversation
🎊 +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. |
🎊 +1 overall
This message was automatically generated. |
@@ -592,7 +594,13 @@ private synchronized void refresh(boolean forceOnline) throws IOException { | |||
|
|||
// This is added to the last of the list so it overwrites the 'default' rsgroup loaded | |||
// from region group table or zk | |||
groupList.add(new RSGroupInfo(RSGroupInfo.DEFAULT_GROUP, getDefaultServers(groupList))); |
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.
Why change this? getDefaultServers not used any more?
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.
getDefaultServers
is still used in behind. This is just for version compatibility, not sure whether groupList has a default group or not after this patch.
return ProtobufUtil.toProtoGroupInfo(rsGroupInfo); | ||
} else { | ||
RSGroupInfo defaultGroupInfo = new RSGroupInfo(rsGroupInfo.getName()); | ||
rsGroupInfo.getConfiguration().forEach(defaultGroupInfo::setConfiguration); |
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.
I didn't follow this. DEFAULT_GROUP don't need this?
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.
Yean, after HBASE-22820, we do not persist default group any more. So in this patch, just persist the configuration of default group. As for servers of default group, it is set while refresh()
.
🎊 +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
@@ -1270,6 +1270,9 @@ public synchronized void renameRSGroup(String oldName, String newName) throws IO | |||
@Override | |||
public synchronized void updateRSGroupConfig(String groupName, Map<String, String> configuration) | |||
throws IOException { | |||
if (RSGroupInfo.DEFAULT_GROUP.equals(groupName)) { | |||
throw new ConstraintException(RSGroupInfo.DEFAULT_GROUP + " can't be stored persistently"); |
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.
Please add more comment here.
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.
OK
🎊 +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. |
🎊 +1 overall
This message was automatically generated. |
No description provided.