joshelser commented on a change in pull request #4052:
URL: https://github.com/apache/hbase/pull/4052#discussion_r830672891
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -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());
Review 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.
##########
File path: hbase-shell/src/main/ruby/shell/commands/add_peer.rb
##########
@@ -34,11 +34,14 @@ def help
to the peer cluster.
An optional parameter for table column families identifies which tables and/or
column families
will be replicated to the peer cluster.
+An optional parameter for the boolean operator to be applied over different
WAL Entry filters. If
+omitted, conjunction (AND) is applied.
Review 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.
##########
File path: hbase-shell/src/main/ruby/hbase/replication_admin.rb
##########
@@ -67,6 +67,7 @@ def add_peer(id, args = {}, peer_tableCFs = nil)
peer_state = args.fetch(STATE, nil)
remote_wal_dir = args.fetch(REMOTE_WAL_DIR, nil)
serial = args.fetch(SERIAL, nil)
+ chain_operator = args.fetch(OPERATOR, nil)
Review comment:
```suggestion
chain_operator = args.fetch(OPERATOR, nil).upcase
```
To normalize `chain_operator`. Ease of use if an operator types `and`
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ChainWALEntryFilter.java
##########
@@ -56,6 +59,13 @@ public ChainWALEntryFilter(List<WALEntryFilter> filters) {
initCellFilters();
}
+ public ChainWALEntryFilter(List<WALEntryFilter> filters, String
operatorName) {
+ this(filters);
+ if (!StringUtils.isEmpty(operatorName)) {
+ this.operator = Operator.valueOf(operatorName);
Review 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).
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/replication/NamespaceTableCfWALEntryFilter.java
##########
@@ -56,8 +56,9 @@ public Cell filterCell(final Entry entry, Cell cell) {
if (CellUtil.matchingColumn(cell, WALEdit.METAFAMILY, WALEdit.BULK_LOAD)) {
// If the cell is about BULKLOAD event, unpack and filter it by
BulkLoadCellFilter.
return bulkLoadFilter.filterCell(cell, fam ->
!peerConfig.needToReplicate(tableName, fam));
- } else {
+ } else if ((!CellUtil.matchingFamily(cell, WALEdit.METAFAMILY))){
Review comment:
```suggestion
} else if (!CellUtil.matchingFamily(cell, WALEdit.METAFAMILY)) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
