Re: [PR] HBASE-29301: Fix AggregrateImplemention pagination logic [hbase]
Apache-HBase commented on PR #6978:
URL: https://github.com/apache/hbase/pull/6978#issuecomment-2867760311
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|::|--:|:|::|:---:|
| +0 :ok: | reexec | 0m 41s | | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | | Unprocessed flag(s):
--brief-report-file --spotbugs-strict-precheck --author-ignore-list
--blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck |
_ Prechecks _ |
_ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 0s | | master passed |
| +1 :green_heart: | compile | 1m 37s | | master passed |
| +1 :green_heart: | javadoc | 0m 54s | | master passed |
| +1 :green_heart: | shadedjars | 8m 15s | | branch has no errors when
building our shaded downstream artifacts. |
_ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 18s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 52s | | the patch passed |
| +1 :green_heart: | compile | 1m 48s | | the patch passed |
| +1 :green_heart: | javac | 1m 48s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 59s | | the patch passed |
| +1 :green_heart: | shadedjars | 8m 11s | | patch has no errors when
building our shaded downstream artifacts. |
_ Other Tests _ |
| -1 :x: | unit | 274m 24s |
[/patch-unit-hbase-server.txt](https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6978/1/artifact/yetus-jdk17-hadoop3-check/output/patch-unit-hbase-server.txt)
| hbase-server in the patch failed. |
| +1 :green_heart: | unit | 4m 0s | | hbase-endpoint in the patch
passed. |
| | | 314m 19s | | |
| Subsystem | Report/Notes |
|--:|:-|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base:
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6978/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
|
| GITHUB PR | https://github.com/apache/hbase/pull/6978 |
| JIRA Issue | HBASE-29301 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux 7de0a1fb57b2 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May
23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / d4e9d3ceac5991c894f9a180f3aee6a96c9085d7 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results |
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6978/1/testReport/
|
| Max. process+thread count | 4449 (vs. ulimit of 3) |
| modules | C: hbase-server hbase-endpoint U: . |
| Console output |
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6978/1/console
|
| versions | git=2.34.1 maven=3.9.8 |
| Powered by | Apache Yetus 0.15.0 https://yetus.apache.org |
This message was automatically generated.
--
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]
Re: [PR] HBASE-29301: Fix AggregrateImplemention pagination logic [hbase]
charlesconnell commented on code in PR #6978:
URL: https://github.com/apache/hbase/pull/6978#discussion_r2081822422
##
hbase-endpoint/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java:
##
@@ -560,20 +609,22 @@ private void postScanPartialResultUpdate(List
results, PartialResultContex
}
}
- private void setPartialResultResponse(AggregateResponse.Builder builder,
AggregateRequest request,
-boolean hasMoreRows, PartialResultContext context) throws IOException {
-// If we encountered an RpcThrottlingException, tell the client the
partial result we've
-// accumulated so far, and what row to start scanning at in order to
finish the scan.
+ private AggregateResponse.Builder responseBuilder(AggregateRequest request,
boolean hasMoreRows,
+PartialResultContext context) {
+AggregateResponse.Builder builder = AggregateResponse.newBuilder();
if (request.getClientSupportsPartialResult() && hasMoreRows) {
if (context.lastRowSuccessfullyProcessedArray != null) {
byte[] lastRowSuccessfullyProcessed = Arrays.copyOfRange(
context.lastRowSuccessfullyProcessedArray,
context.lastRowSuccessfullyProcessedOffset,
context.lastRowSuccessfullyProcessedOffset +
context.lastRowSuccessfullyProcessedLength);
builder.setNextChunkStartRow(ByteString.copyFrom(
ClientUtil.calculateTheClosestNextRowKeyForPrefix(lastRowSuccessfullyProcessed)));
+ } else {
+builder.setNextChunkStartRow(request.getScan().getStartRow());
Review Comment:
This is the key fix here. If no results were found, indicate that retry is
still necessary
##
hbase-endpoint/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java:
##
@@ -560,20 +609,22 @@ private void postScanPartialResultUpdate(List
results, PartialResultContex
}
}
- private void setPartialResultResponse(AggregateResponse.Builder builder,
AggregateRequest request,
-boolean hasMoreRows, PartialResultContext context) throws IOException {
-// If we encountered an RpcThrottlingException, tell the client the
partial result we've
-// accumulated so far, and what row to start scanning at in order to
finish the scan.
+ private AggregateResponse.Builder responseBuilder(AggregateRequest request,
boolean hasMoreRows,
+PartialResultContext context) {
+AggregateResponse.Builder builder = AggregateResponse.newBuilder();
if (request.getClientSupportsPartialResult() && hasMoreRows) {
if (context.lastRowSuccessfullyProcessedArray != null) {
byte[] lastRowSuccessfullyProcessed = Arrays.copyOfRange(
context.lastRowSuccessfullyProcessedArray,
context.lastRowSuccessfullyProcessedOffset,
context.lastRowSuccessfullyProcessedOffset +
context.lastRowSuccessfullyProcessedLength);
builder.setNextChunkStartRow(ByteString.copyFrom(
ClientUtil.calculateTheClosestNextRowKeyForPrefix(lastRowSuccessfullyProcessed)));
+ } else {
+builder.setNextChunkStartRow(request.getScan().getStartRow());
Review Comment:
This is the key fix here. If no results were found, indicate that retry is
still necessary.
--
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]
Re: [PR] HBASE-29301: Fix AggregrateImplemention pagination logic [hbase]
Apache-HBase commented on PR #6978:
URL: https://github.com/apache/hbase/pull/6978#issuecomment-2867059712
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|::|--:|:|::|:---:|
| +0 :ok: | reexec | 0m 43s | | Docker mode activated. |
_ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | | No case conflicting files
found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available.
|
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain
any @author tags. |
| +1 :green_heart: | hbaseanti | 0m 0s | | Patch does not have any
anti-patterns. |
_ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 52s | | master passed |
| +1 :green_heart: | compile | 5m 21s | | master passed |
| +1 :green_heart: | checkstyle | 1m 8s | | master passed |
| +1 :green_heart: | spotbugs | 3m 10s | | master passed |
| +1 :green_heart: | spotless | 1m 16s | | branch has no errors when
running spotless:check. |
_ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 34s | | the patch passed |
| +1 :green_heart: | compile | 4m 59s | | the patch passed |
| +1 :green_heart: | javac | 4m 59s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks
issues. |
| +1 :green_heart: | checkstyle | 1m 5s | | the patch passed |
| +1 :green_heart: | spotbugs | 3m 29s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 17m 38s | | Patch does not cause any
errors with Hadoop 3.3.6 3.4.0. |
| +1 :green_heart: | spotless | 1m 10s | | patch has no errors when
running spotless:check. |
_ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 31s | | The patch does not
generate ASF License warnings. |
| | | 61m 13s | | |
| Subsystem | Report/Notes |
|--:|:-|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base:
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6978/1/artifact/yetus-general-check/output/Dockerfile
|
| GITHUB PR | https://github.com/apache/hbase/pull/6978 |
| JIRA Issue | HBASE-29301 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell
detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux d6be83b35b95 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May
23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / d4e9d3ceac5991c894f9a180f3aee6a96c9085d7 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 84 (vs. ulimit of 3) |
| modules | C: hbase-server hbase-endpoint U: . |
| Console output |
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6978/1/console
|
| versions | git=2.34.1 maven=3.9.8 spotbugs=4.7.3 |
| Powered by | Apache Yetus 0.15.0 https://yetus.apache.org |
This message was automatically generated.
--
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]
Re: [PR] HBASE-29301: Fix AggregrateImplemention pagination logic [hbase]
charlesconnell commented on code in PR #6978:
URL: https://github.com/apache/hbase/pull/6978#discussion_r2081822422
##
hbase-endpoint/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java:
##
@@ -560,20 +609,22 @@ private void postScanPartialResultUpdate(List
results, PartialResultContex
}
}
- private void setPartialResultResponse(AggregateResponse.Builder builder,
AggregateRequest request,
-boolean hasMoreRows, PartialResultContext context) throws IOException {
-// If we encountered an RpcThrottlingException, tell the client the
partial result we've
-// accumulated so far, and what row to start scanning at in order to
finish the scan.
+ private AggregateResponse.Builder responseBuilder(AggregateRequest request,
boolean hasMoreRows,
+PartialResultContext context) {
+AggregateResponse.Builder builder = AggregateResponse.newBuilder();
if (request.getClientSupportsPartialResult() && hasMoreRows) {
if (context.lastRowSuccessfullyProcessedArray != null) {
byte[] lastRowSuccessfullyProcessed = Arrays.copyOfRange(
context.lastRowSuccessfullyProcessedArray,
context.lastRowSuccessfullyProcessedOffset,
context.lastRowSuccessfullyProcessedOffset +
context.lastRowSuccessfullyProcessedLength);
builder.setNextChunkStartRow(ByteString.copyFrom(
ClientUtil.calculateTheClosestNextRowKeyForPrefix(lastRowSuccessfullyProcessed)));
+ } else {
+builder.setNextChunkStartRow(request.getScan().getStartRow());
Review Comment:
This is half of the key fix here. If no results were found and
getClientSupportsPartialResult, indicate that retry is still necessary.
--
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]
Re: [PR] HBASE-29301: Fix AggregrateImplemention pagination logic [hbase]
charlesconnell commented on code in PR #6978:
URL: https://github.com/apache/hbase/pull/6978#discussion_r2081846624
##
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScannerImpl.java:
##
@@ -59,7 +59,7 @@
* RegionScannerImpl is used to combine scanners from multiple Stores (aka
column families).
*/
@InterfaceAudience.Private
-class RegionScannerImpl implements RegionScanner, Shipper, RpcCallback {
Review Comment:
Had to do this for my unit test
--
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]
Re: [PR] HBASE-29301: Fix AggregrateImplemention pagination logic [hbase]
charlesconnell commented on code in PR #6978:
URL: https://github.com/apache/hbase/pull/6978#discussion_r2081822422
##
hbase-endpoint/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java:
##
@@ -560,20 +609,22 @@ private void postScanPartialResultUpdate(List
results, PartialResultContex
}
}
- private void setPartialResultResponse(AggregateResponse.Builder builder,
AggregateRequest request,
-boolean hasMoreRows, PartialResultContext context) throws IOException {
-// If we encountered an RpcThrottlingException, tell the client the
partial result we've
-// accumulated so far, and what row to start scanning at in order to
finish the scan.
+ private AggregateResponse.Builder responseBuilder(AggregateRequest request,
boolean hasMoreRows,
+PartialResultContext context) {
+AggregateResponse.Builder builder = AggregateResponse.newBuilder();
if (request.getClientSupportsPartialResult() && hasMoreRows) {
if (context.lastRowSuccessfullyProcessedArray != null) {
byte[] lastRowSuccessfullyProcessed = Arrays.copyOfRange(
context.lastRowSuccessfullyProcessedArray,
context.lastRowSuccessfullyProcessedOffset,
context.lastRowSuccessfullyProcessedOffset +
context.lastRowSuccessfullyProcessedLength);
builder.setNextChunkStartRow(ByteString.copyFrom(
ClientUtil.calculateTheClosestNextRowKeyForPrefix(lastRowSuccessfullyProcessed)));
+ } else {
+builder.setNextChunkStartRow(request.getScan().getStartRow());
Review Comment:
This is half of the key fix here. If no results were found, indicate that
retry is still necessary.
--
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]
Re: [PR] HBASE-29301: Fix AggregrateImplemention pagination logic [hbase]
charlesconnell commented on code in PR #6978:
URL: https://github.com/apache/hbase/pull/6978#discussion_r2081837521
##
hbase-endpoint/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java:
##
@@ -111,11 +111,16 @@ public void getMax(RpcController controller,
AggregateRequest request,
postScanPartialResultUpdate(results, partialResultContext);
results.clear();
} while (hasMoreRows);
- if (max != null) {
-AggregateResponse.Builder builder = AggregateResponse.newBuilder();
-builder.addFirstPart(ci.getProtoForCellType(max).toByteString());
-setPartialResultResponse(builder, request, hasMoreRows,
partialResultContext);
-response = builder.build();
+ if (max != null && !request.getClientSupportsPartialResult()) {
+ByteString first = ci.getProtoForCellType(max).toByteString();
+response = AggregateResponse.newBuilder().addFirstPart(first).build();
+ } else if (request.getClientSupportsPartialResult()) {
+AggregateResponse.Builder responseBuilder =
+ responseBuilder(request, hasMoreRows, partialResultContext);
+if (max != null) {
+
responseBuilder.addFirstPart(ci.getProtoForCellType(max).toByteString());
+}
+response = responseBuilder.build();
Review Comment:
I'm making a call here about the API contract:
- If no results were found, and partial results are not supported, return
null. This maintains legacy behavior from before I get involved in the
AggregationClient.
- If no results were found, and partial results are supported, return a
non-null object with the `firstPart` list empty. This is a new behavior, one
that the client can already handle correctly.
--
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]
Re: [PR] HBASE-29301: Fix AggregrateImplemention pagination logic [hbase]
charlesconnell commented on code in PR #6978:
URL: https://github.com/apache/hbase/pull/6978#discussion_r2081837521
##
hbase-endpoint/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java:
##
@@ -111,11 +111,16 @@ public void getMax(RpcController controller,
AggregateRequest request,
postScanPartialResultUpdate(results, partialResultContext);
results.clear();
} while (hasMoreRows);
- if (max != null) {
-AggregateResponse.Builder builder = AggregateResponse.newBuilder();
-builder.addFirstPart(ci.getProtoForCellType(max).toByteString());
-setPartialResultResponse(builder, request, hasMoreRows,
partialResultContext);
-response = builder.build();
+ if (max != null && !request.getClientSupportsPartialResult()) {
+ByteString first = ci.getProtoForCellType(max).toByteString();
+response = AggregateResponse.newBuilder().addFirstPart(first).build();
+ } else if (request.getClientSupportsPartialResult()) {
+AggregateResponse.Builder responseBuilder =
+ responseBuilder(request, hasMoreRows, partialResultContext);
+if (max != null) {
+
responseBuilder.addFirstPart(ci.getProtoForCellType(max).toByteString());
+}
+response = responseBuilder.build();
Review Comment:
I'm making a call here about the API contract:
- If no results were found, and partial results are not supported, return
null. This maintains legacy behavior from before I get involved in the
AggregationClient.
- If no results were found, and partial results are supported, return a
non-null object with the `firstPart` list empty. This is a new behavior, one
that the client can already handle correctly.
I mirror this approach in getMax, getMin, getSum, getStd, and getMedian.
getRowNum works differently.
--
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]
Re: [PR] HBASE-29301: Fix AggregrateImplemention pagination logic [hbase]
charlesconnell commented on code in PR #6978: URL: https://github.com/apache/hbase/pull/6978#discussion_r2081829553 ## hbase-endpoint/src/main/java/org/apache/hadoop/hbase/coprocessor/AggregateImplementation.java: ## @@ -264,7 +279,7 @@ public void getRowNum(RpcController controller, AggregateRequest request, List results = new ArrayList<>(); InternalScanner scanner = null; PartialResultContext partialResultContext = new PartialResultContext(); -boolean hasMoreRows = false; +boolean hasMoreRows = true; Review Comment: This is half of the key fix here. Assume there are more rows to be found until a scanner tells us otherwise. -- 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]
