Uh oh!
There was an error while loading. Please reload this page.
HBASE-25955 Setting NAMESPACES when adding a replication peer doesn't… - #3347
Closed
wchevreuil wants to merge 4 commits into
Closed
HBASE-25955 Setting NAMESPACES when adding a replication peer doesn't…#3347wchevreuil wants to merge 4 commits into
wchevreuil wants to merge 4 commits into
Conversation
wchevreuil
requested review from
Apache9, apurtell, bharathv, ddupg, petersomogyi and xcangCRMJune 1, 2021 10:18
Apache-HBase
commented
Jun 1, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 1, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 1, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 1, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 1, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 1, 2021
💔 -1 overall
This message was automatically generated. |
wchevreuil
commented
Jun 1, 2021
ContributorAuthor
Looking this UT failure on this TestRemoveFromSerialReplicationPeer. |
| * return null if they don't want this filter */ | ||
| protected WALEntryFilter getNamespaceTableCfWALEntryFilter() { | ||
| //If none of the below sets are defined, there's no reason to create this filter | ||
| if(ctx.getPeerConfig().getNamespaces()==null && ctx.getPeerConfig().getTableCFsMap()==null |
Contributor
There was a problem hiding this comment.
Should we handle the empty values as well?Strings.isNullOrEmpty(strVar)
ContributorAuthor
There was a problem hiding this comment.
Ack! Switched to CollectionUtils.isEmpty and MapUtils.isEmpty.
| .setReplicateAllUserTables(false) | ||
| // sets namespaces | ||
| .setNamespaces(namespaces).build(); | ||
| hbaseAdmin.addReplicationPeer("testNamespacesMutualExclusiveScopesWALEntryFilter", rpc); |
Contributor
There was a problem hiding this comment.
mind making testNamespacesMutualExclusiveScopesWALEntryFilter a constant string in the test?
ContributorAuthor
There was a problem hiding this comment.
Switched to TestName usage.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| WALEntryFilter tableCfFilter = getNamespaceTableCfWALEntryFilter(); | ||
| if (tableCfFilter != null) { | ||
| filters.add(tableCfFilter); | ||
| } else { |
Contributor
There was a problem hiding this comment.
This means when any of the wildcard kind of setting (like one NS for include) is present, a per table:cf setting wont get honoured?
To be specific say we have 2 NS, ns1 and ns2. Now ns1 is in the include NS list. So all tables in ns1 to be replicated. Thats it. ns2 is neither in include list nor in exclude list. But ns2 having one table with CFs of replication scope = 1. But as per this change we will not consider those tables for replication as scopeFilter wont come in then?
ContributorAuthor
There was a problem hiding this comment.
That would be the case. The given table/CF should be in the namespace passed, or in the TableCFsMap defined in the addPeer command. It does break compatibility, but it's still possible to achieve the same level of granularity by using a combination of ExcludeTableCFsMap/ExcludeNamespaces/namespaces/TableCFsMap, which is much more convenient than having to alter table schema (which also requires table to be offline).
Contributor
There was a problem hiding this comment.
Any way we can avoid the compatibility break? May be a bigger change but can we think ways? The WALEntryFilter s to act in an OR fashion? The chain is more or less an AND model.
ContributorAuthor
There was a problem hiding this comment.
Maybe come up with an extra, namespace only filter, then?
wchevreuil
commented
Jun 2, 2021
Comment on lines
+96
to
100
| ReplicationPeerConfig.newBuilder(peerConfig) | ||
| .setTableCFsMap(Collections.emptyMap()) | ||
| .setReplicateAllUserTables(true) | ||
| .setExcludeTableCFsMap(ImmutableMap.of(tableName, Collections.emptyList())).build()); | ||
ContributorAuthor
There was a problem hiding this comment.
The previous test behaviour isn't valid anymore, as it was relying on an empty namespaces collection to filter out entries, but now, passing null or empty namespaces just activates the scoped filter, which will cause these table entries to be picked for replication, as its scope is set to 1.
Apache-HBase
commented
Jun 2, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 2, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 2, 2021
💔 -1 overall
This message was automatically generated. |
wchevreuil
commented
Jan 21, 2022
ContributorAuthor
Closing this one in favour of a different approach that doesn't change current behaviour. |
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
… have any effect