Copilot commented on code in PR #7079:
URL: https://github.com/apache/hbase/pull/7079#discussion_r2160046479
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestClientOperationTimeout.java:
##########
@@ -321,6 +322,70 @@ public void testMultiGetRetryTimeout() {
}
}
+ /**
+ * Test that for a batch operation where region location resolution fails
for the first action in
+ * the batch and consumes the entire operation timeout, that the location
error is preserved for
+ * the first action and that the rest of the batch is failed fast with
+ * OperationTimeoutExceededException , this also (indirectly) tests that the
action counter is
+ * decremented properly for all actions, see last catch block
+ */
+ @Test
+ public void testMultiOperationTimoutWithLocationError() throws IOException,
InterruptedException {
Review Comment:
There is a typo in the test method name
'testMultiOperationTimoutWithLocationError'. Consider renaming it to
'testMultiOperationTimeoutWithLocationError' for clarity.
```suggestion
public void testMultiOperationTimeoutWithLocationError() throws
IOException, InterruptedException {
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRequestFutureImpl.java:
##########
@@ -448,30 +448,22 @@ void groupAndSendMultiAction(List<Action> currentActions,
int numAttempt) {
boolean isReplica = false;
List<Action> unknownReplicaActions = null;
+ List<Action> locateRegionFailedActions = null;
for (Action action : currentActions) {
if (isOperationTimeoutExceeded()) {
- String message = numAttempt == 1
- ? "Operation timeout exceeded during resolution of region locations,
"
- + "prior to executing any actions."
- : "Operation timeout exceeded during re-resolution of region
locations on retry "
- + (numAttempt - 1) + ".";
-
- message += " Meta may be slow or operation timeout too short for batch
size or retries.";
- OperationTimeoutExceededException exception =
- new OperationTimeoutExceededException(message);
-
- // Clear any actions we already resolved, because none will have been
executed yet
- // We are going to fail all passed actions because there's no way we
can execute any
- // if operation timeout is exceeded.
actionsByServer.clear();
- for (Action actionToFail : currentActions) {
- manageLocationError(actionToFail, exception);
- }
+ failIncompleteActionsWithOpTimeout(currentActions,
locateRegionFailedActions, numAttempt);
return;
}
RegionLocations locs = findAllLocationsOrFail(action, true);
- if (locs == null) continue;
+ if (locs == null) {
+ if (locateRegionFailedActions == null) {
+ locateRegionFailedActions = new ArrayList<>(1);
+ }
Review Comment:
[nitpick] The null check and initialization for 'locateRegionFailedActions'
is repeated in multiple places. Consider extracting this logic into a helper
method to reduce duplication and improve maintainability.
```suggestion
locateRegionFailedActions =
initializeIfNull(locateRegionFailedActions);
```
--
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]
