Copilot commented on code in PR #7577:
URL: https://github.com/apache/hbase/pull/7577#discussion_r2651682076
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeerConfig.java:
##########
@@ -140,6 +142,10 @@ public boolean isSerial() {
return serial;
}
Review Comment:
The public getter method getSleepForRetries() is missing Javadoc
documentation. For consistency with other public methods in this class and to
help users understand the purpose and behavior of this configuration property,
add Javadoc that explains:
- What this value represents (sleep time between retries in milliseconds)
- The default value (0, which means use global configuration)
- When it's used (during replication retries)
```suggestion
/**
* Returns the per-peer sleep time between replication retries, in
milliseconds.
* <p>
* A value of {@code 0} means that this peer does not define its own retry
sleep and the
* global replication configuration should be used instead.
* This value is consulted when performing replication retries for this
peer.
*
* @return sleep time between replication retries in milliseconds, or
{@code 0} to use the
* global configuration
*/
```
##########
hbase-shell/src/main/ruby/shell/commands/set_peer_sleep_for_retries.rb:
##########
@@ -0,0 +1,42 @@
+#
+# Copyright The Apache Software Foundation
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+module Shell
+ module Commands
+ class SetPeerSleepForRetries < Command
+ def help
+ <<-EOF
+Set the replication source sleep time between retries for the specified peer.
+Examples:
+
+ # set sleep time to 2 seconds (2000ms) between retries for a peer
+ hbase> set_peer_sleep_for_retries '1', 2000
+ # unset sleep time for a peer to use the global default configured in
server-side
Review Comment:
The comment states "unset sleep time for a peer to use the global default
configured in server-side" when passing 0, but this behavior differs from the
bandwidth configuration pattern shown in the same file. In the bandwidth test
(lines 609-622), 0 is also used as the initial/default value, but the bandwidth
implementation treats 0 as "use default bandwidth". For consistency and
clarity, consider documenting this explicitly in the help text that 0 has
special meaning as a sentinel value for "use global default", and ensure users
understand that setting it to 0 is not the same as setting it to the actual
configured global default value.
```suggestion
A value of 0 is treated specially as a sentinel meaning "use the global
default
sleep time configured on the server side" rather than an explicit sleep time.
Examples:
# set sleep time to 2 seconds (2000ms) between retries for a peer
hbase> set_peer_sleep_for_retries '1', 2000
# unset the peer-specific sleep time so that this peer uses the global
default
# configured on the server side (0 is a sentinel and does not change that
default)
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java:
##########
@@ -526,6 +522,19 @@ private long getCurrentBandwidth() {
return peerBandwidth != 0 ? peerBandwidth : defaultBandwidth;
}
+ /**
+ * Get the sleep time for retries. Check peer config first, if set use it,
otherwise fall back to
+ * global configuration.
+ * @return sleep time in milliseconds
+ */
Review Comment:
This method overrides [ReplicationSourceInterface.getSleepForRetries](1); it
is advisable to add an Override annotation.
```suggestion
*/
@Override
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java:
##########
@@ -526,6 +522,19 @@ private long getCurrentBandwidth() {
return peerBandwidth != 0 ? peerBandwidth : defaultBandwidth;
}
+ /**
+ * Get the sleep time for retries. Check peer config first, if set use it,
otherwise fall back to
+ * global configuration.
+ * @return sleep time in milliseconds
+ */
+ public long getSleepForRetries() {
+ long peerSleepForRetries =
replicationPeer.getPeerConfig().getSleepForRetries();
+ if (peerSleepForRetries > 0) {
+ return peerSleepForRetries;
+ }
+ return this.conf.getLong("replication.source.sleepforretries", 1000);
+ }
Review Comment:
The fallback logic is inconsistent with the existing `getCurrentBandwidth()`
pattern. The bandwidth implementation (lines 519-523) treats 0 as "use
default", but this implementation treats values > 0 as valid peer settings and
0 as "use default". This creates an inconsistency where setting sleepForRetries
to 0 means "use global default" but for bandwidth, 0 means "use default".
Consider using the same pattern as bandwidth (checking != 0 instead of > 0) for
consistency, or add explicit documentation about this behavior difference.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java:
##########
@@ -526,6 +522,19 @@ private long getCurrentBandwidth() {
return peerBandwidth != 0 ? peerBandwidth : defaultBandwidth;
}
+ /**
+ * Get the sleep time for retries. Check peer config first, if set use it,
otherwise fall back to
+ * global configuration.
+ * @return sleep time in milliseconds
+ */
+ public long getSleepForRetries() {
+ long peerSleepForRetries =
replicationPeer.getPeerConfig().getSleepForRetries();
+ if (peerSleepForRetries > 0) {
+ return peerSleepForRetries;
+ }
+ return this.conf.getLong("replication.source.sleepforretries", 1000);
+ }
Review Comment:
Missing test coverage for the new getSleepForRetries() method and its
fallback logic. While the existing tests mock getSleepForRetries() to return
0L, there's no test that verifies:
1. When peer config has a positive value (e.g., 2000), it returns that value
2. When peer config is 0, it falls back to the global configuration value
3. The integration between peer-level and global configuration
Consider adding a dedicated unit test in TestReplicationSource.java that
verifies this fallback behavior, similar to how bandwidth is tested in other
parts of the codebase.
--
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]
