-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29236: Add Support for Dynamic Configuration at the Coprocessor Level #6931
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-29236: Add Support for Dynamic Configuration at the Coprocessor Level #6931
Conversation
8bda4a3 to
37a7919
Compare
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
anmolnar
left a comment
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.
Patch looks good to me. Thanks @kgeisz !
I think this is the point that you should work together with @sharmaar12 and add unit test which starts up a non-RO mini cluster, dynamically changes it to RO mode and validate some mutatation commands.
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Kota-SH
left a comment
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.
LGTM, just a couple of nits.
+1 for the unit tests.
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
Show resolved
Hide resolved
5fa6a0b to
b9a05d8
Compare
|
@anmolnar, I have added some unit test cases. I also did a couple more things:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
anmolnar
left a comment
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.
lgtm. Thanks for the tests!
| if (edit.isMetaEdit() || edit.isEmpty()) { | ||
| return; | ||
| } |
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.
So, you don't need these checks anymore, because you start the mini cluster in R/W mode in your tests. Does that mean a real cluster can be started in R/O mode, have you verified that?
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.
@anmolnar, I cannot start HBase in Read-Only mode with these current changes.
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.
@anmolnar, I made it so prePut() and preBatchMutate() will skip the internalReadOnlyGuard() check if the table is a system table, rather than if the WALEdit is empty or a meta edit. I also added a unit test class that verifies HBase can be started in Read-Only mode.
This comment has been minimized.
This comment has been minimized.
Change-Id: If6e6104eca7937a0a478cb319bafefa5aefec708
…as actually changed Change-Id: I552c429df8f65e843de554fd8624854821b75c4e
…rs() Change-Id: If19fb7d206f22d0950e97d6b65b233092e11a13a
…led with Region coprocessors Change-Id: Ie34bbd973fab001bde86c29c35e7a3d89db395b8
Change-Id: I0c334c5fa85c0ba1b315705bd453584a96664009
….global.readonly.enabled Change-Id: If688376a44eceebb7e2829781e71a30b9985eb6b
Change-Id: Idf2432c7752a53c229d0d0dcaadb3582087635fe
Change-Id: I56f866ea20e585606d1d9102f815f008863e7c67
Change-Id: I5f4f57b63a8707d00dc67abc912abf4d9f1a7959
5e2854e to
035cfdb
Compare
This comment has been minimized.
This comment has been minimized.
…tchMutate() when working with a system table Change-Id: I84019e536dc2813d68eb480a12791787ddfe4cf3
…nly mode Change-Id: I64904d94bad6ada43c4a49fe6b2eaf3193db34e0
This comment has been minimized.
This comment has been minimized.
anmolnar
left a comment
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.
great stuff! thanks @kgeisz !
| TEST_UTIL.deleteTable(TEST_TABLE); | ||
| connection.close(); | ||
| TEST_UTIL.shutdownMiniCluster(); | ||
| throw new RuntimeException(e); |
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 hope this would not cause the whole test pipeline to fail?
|
Maybe it is worth pondering if we should rename this JIRA to highlight the fact that it actually also adds support for dynamic conf at coprocessor level, which may expose it to others leading to several other use cases, with maybe R/O flag being an example implementation? I am fine even if we break this into 2 jiras to expose the same. Anyways overall LGTM, +1 from me as well. Nice work! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@NihalJain, thanks for the feedback! I updated the name of the Jira ticket and added some info to the ticket's description. I also updated the name of the PR to match the Jira ticket. |
|
🎊 +1 overall
This message was automatically generated. |
|
Build timed out, but it was successful, so I merge the patch regardless. Thanks @kgeisz ! |
… Level (#6931) Co-authored-by: Andor Molnar <[email protected]> Co-authored-by: Anuj Sharma <[email protected]>
… Level (#6931) Co-authored-by: Andor Molnar <[email protected]> Co-authored-by: Anuj Sharma <[email protected]>
https://issues.apache.org/jira/browse/HBASE-29236
This pull request adds support for Dynamic Configuration at the coprocessor level and makes the
hbase.global.readonly.enabledconfig variable dynamically configurable. This variable is used with the ReadOnlyController coprocessor. In order to make this class dynamically configurable, we have it implement the ConfigurationObserver interface. We also need to register this observer with the ConfigurationManager, so the ReadOnlyController is registered to HMaster's, HRegionServer's, and HRegion's ConfigurationManager. Also, since the ReadOnlyController is a coprocessor, we need to use the CoprocessorHost in order to get the coprocessor and register it with the ConfigurationManager.hbase.global.readonly.enabledcan be configured dynamically in the following way:hbase.global.readonly.enabledto hbase-site.xml:hbase.global.readonly.enabledfromfalsetotruein hbase-site.xml and save the file.update_all_config.