HBASE-25955 Setting NAMESPACES when adding a replication peer still requires scope definition at CF level #4052
+176
−19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
TestFromClientSide5 pass locally for me. Flakey? |
joshelser
requested changes
Mar 20, 2022
Comment on lines
+37
to
+38
| An optional parameter for the boolean operator to be applied over different WAL Entry filters. If | ||
| omitted, conjunction (AND) is applied. |
There was a problem hiding this comment.
I'm wondering if, through the shell, we should provide some more simplicity for the operator. They are unaware of any of the WALFilters that we are setting behind the scenes. To them, this operator would be nothing more than a "magic word" (e.g. "I put 'OR' and then my data gets replicated"). I guess it's better to get this code committed and then think about ways to make it more clear to admins.
There was a problem hiding this comment.
Indeed this ended up too "programming oriented". Maybe we could change to a more meaningful boolean property, such as: "PASS_ONE_FILTER_ONLY"?
| @@ -290,12 +290,14 @@ public static ReplicationPeerConfig convert(ReplicationProtos.ReplicationPeer pe | |||
| peer.getTableCfsList().toArray(new ReplicationProtos.TableCF[peer.getTableCfsCount()])); | |||
| if (tableCFsMap != null) { | |||
| builder.setTableCFsMap(tableCFsMap); | |||
| builder.setChainedFiltersOperation(peer.getChainOperator()); | |||
There was a problem hiding this comment.
Should apply validation here (in addition to or instead of Ruby) as the Java API to set the chainOperator as a user could be writing Java code directly instead of writing Ruby code to interact with HBase.
There was a problem hiding this comment.
We could add some validation here, but none of the other already existing fields are doing much validation either. An invalid value would fail the addPeer operation later on ChainWALEntryFilter constructor, as the enum "valueOf" call would raise an exception.
| public ChainWALEntryFilter(List<WALEntryFilter> filters, String operatorName) { | ||
| this(filters); | ||
| if (!StringUtils.isEmpty(operatorName)) { | ||
| this.operator = Operator.valueOf(operatorName); |
There was a problem hiding this comment.
I was trying to figure out the first place we read the String "operatorName" and make sure it fails gracefully.
I know you have the client-side checking in Ruby code, and I suggested we have Java data validation. We should check it here as future-proofing. I think this happens early enough in the replication setup that the client would see a RemoteException flowing back to them? (not that their add_peer call would succeed and just not replicate any data because it failed).
There was a problem hiding this comment.
You mean, beyond the checks to avoid an NPE, explicitly extra check for the valid strings and throw IllegalArgumentException, rather than letting the enum error?
There was a problem hiding this comment.
Yeah, something that we would at least throw back a well-formed exception (and not something that might be very terse/short)
...server/src/main/java/org/apache/hadoop/hbase/replication/NamespaceTableCfWALEntryFilter.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Josh Elser <[email protected]>
…/NamespaceTableCfWALEntryFilter.java Co-authored-by: Josh Elser <[email protected]>
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Jul 1, 2022
| @@ -290,12 +290,14 @@ public static ReplicationPeerConfig convert(ReplicationProtos.ReplicationPeer pe | |||
| peer.getTableCfsList().toArray(new ReplicationProtos.TableCF[peer.getTableCfsCount()])); | |||
| if (tableCFsMap != null) { | |||
| builder.setTableCFsMap(tableCFsMap); | |||
| builder.setChainedFiltersOperation(peer.getChainOperator()); | |||
There was a problem hiding this comment.
I do not think this is a good name? The chain filter is an internal implementation in HBase, maybe in the future we could change the implementation to not use a filter...
Maybe just name it overrideReplicationScope or something similar?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As mentioned on the jira description, setting either NAMESPACES or TABLECFs when calling add_peer still doesn't suffice to allow entries for the related namespaces/tables to be replicated, if those don't have replication scope set to '1' in the CF descriptor.
The above happens because ChainWalEntryFilter, currently, applies conjunction (AND) to all its chained filters. Following suggestions from @anoopsjohn on an previous PR, rather than changing this behaviour completely, this PR introduces an option to define between conjunction (AND) or disjunction (OR) logic for ChainWalEntryFilter, keeping current logic (AND) the default one, if not specified.