You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/04/11 15:34:19 UTC
[GitHub] [beam] nielm opened a new pull request, #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
nielm opened a new pull request, #17335:
URL: https://github.com/apache/beam/pull/17335
Fixes issues with the ServiceCallMetrics in SpannerIO:
- Fixes metric resource identifiers:
- `//spanner.googleapis.com/projects/{projectId}/instances/{instanceId}/databases/{databaseId}/tables/{tableId}`
- `//spanner.googleapis.com/projects/{projectId}/instances/{instanceId}/queries/{queryName}`
- Ensure a non-null value for `{queryName}`
- Fix metrics generation in `SpannerIO.Read`
- Fix metrics generation in `SpannerIO.Write`
- Add metrics generation to `NaiveSpannerRead`.
- Refactor and improve `SpannerIOReadTest` to add additional coverage and verify metrics values
- Add unit tests for `NaiveSpannerRead` (non-partitioned read)
- Add metrics value verification in `SpannerIOWriteTest`.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1128906877
Ready to merge: PreCommit test failures are unrelated to this PR: [org.apache.beam.sdk.io.elasticsearch.ElasticsearchIOTest.classMethod](https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/22629/testReport/junit/org.apache.beam.sdk.io.elasticsearch/ElasticsearchIOTest/classMethod/)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
aaltay commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1139230548
@nielm - Could you please respond to the open comments?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1128828643
PreCommit failure was unrelated to this PR, rebased and trying again.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1128827895
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
aaltay commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1125443084
What is the next step on this PR?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1095205400
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1126091029
Apologies - next steps were to rebase to master, resolving conflicts, and fix the failing checkstyle test!
Done.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1148396925
re pre-commit failure in `org.apache.beam.sdk.io.gcp.spanner.SpannerIOWriteExceptionHandlingTest.testExceptionHandlingForWriteGrouped`
I saw this occasionally in my local testing when my local machine was under load and have been looking into it. It does seem to be a timing related flake, unrelated to this PR. Will debug and fix separately; raised #21729
```
> is a org.mockito.exceptions.verification.TooManyActualInvocations
Stacktrace was: org.mockito.exceptions.verification.TooManyActualInvocations:
sleeper.sleep(<any long>);
Wanted 9 times:
-> at org.apache.beam.sdk.io.gcp.spanner.SpannerIOWriteExceptionHandlingTest.testExceptionHandlingForWriteGrouped(SpannerIOWriteExceptionHandlingTest.java:222)
But was 10 times:
```
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on a diff in pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on code in PR #17335:
URL: https://github.com/apache/beam/pull/17335#discussion_r884735083
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -377,6 +382,12 @@ public class SpannerIO {
// Multiple of mutation size to use to gather and sort mutations
private static final int DEFAULT_GROUPING_FACTOR = 1000;
+ // Size of caches for read/write ServiceCallMetric objects .
+ // This is a reasonable limit, as for reads, each worker will process very few different table
+ // read requests, and for writes, batching will ensure that write operations for the same
+ // table occur at
+ public static final int METRICS_CACHE_SIZE = 100;
Review Comment:
yes, fixed
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1141052210
> @nielm - Could you please respond to the open comments?
Done - apologies I was on vacation.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1117076910
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1126091273
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on a diff in pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on code in PR #17335:
URL: https://github.com/apache/beam/pull/17335#discussion_r884734870
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -377,6 +382,12 @@ public class SpannerIO {
// Multiple of mutation size to use to gather and sort mutations
private static final int DEFAULT_GROUPING_FACTOR = 1000;
+ // Size of caches for read/write ServiceCallMetric objects .
+ // This is a reasonable limit, as for reads, each worker will process very few different table
+ // read requests, and for writes, batching will ensure that write operations for the same
+ // table occur at
Review Comment:
Fixed
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj merged pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
chamikaramj merged PR #17335:
URL: https://github.com/apache/beam/pull/17335
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1148135483
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1141052535
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on a diff in pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on code in PR #17335:
URL: https://github.com/apache/beam/pull/17335#discussion_r876468653
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -377,6 +382,12 @@ public class SpannerIO {
// Multiple of mutation size to use to gather and sort mutations
private static final int DEFAULT_GROUPING_FACTOR = 1000;
+ // Size of caches for read/write ServiceCallMetric objects .
+ // This is a reasonable limit, as for reads, each worker will process very few different table
+ // read requests, and for writes, batching will ensure that write operations for the same
+ // table occur at
Review Comment:
Incomplete comment.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/BatchSpannerRead.java:
##########
@@ -114,75 +132,62 @@ public void teardown() throws Exception {
@ProcessElement
public void processElement(ProcessContext c) throws Exception {
Transaction tx = c.sideInput(txView);
- BatchReadOnlyTransaction context =
+ BatchReadOnlyTransaction batchTx =
spannerAccessor.getBatchClient().batchReadOnlyTransaction(tx.transactionId());
- for (Partition p : execute(c.element(), context)) {
- c.output(p);
- }
- }
-
- private List<Partition> execute(ReadOperation op, BatchReadOnlyTransaction tx) {
- if (config.getRpcPriority() != null && config.getRpcPriority().get() != null) {
- return executeWithPriority(op, tx, config.getRpcPriority().get());
- } else {
- return executeWithoutPriority(op, tx);
- }
- }
-
- private List<Partition> executeWithoutPriority(ReadOperation op, BatchReadOnlyTransaction tx) {
- // Query was selected.
- if (op.getQuery() != null) {
- return tx.partitionQuery(op.getPartitionOptions(), op.getQuery());
- }
- // Read with index was selected.
- if (op.getIndex() != null) {
- return tx.partitionReadUsingIndex(
- op.getPartitionOptions(),
- op.getTable(),
- op.getIndex(),
- op.getKeySet(),
- op.getColumns());
- }
- // Read from table was selected.
- return tx.partitionRead(
- op.getPartitionOptions(), op.getTable(), op.getKeySet(), op.getColumns());
- }
-
- private List<Partition> executeWithPriority(
- ReadOperation op, BatchReadOnlyTransaction tx, RpcPriority rpcPriority) {
- // Query was selected.
- if (op.getQuery() != null) {
- return tx.partitionQuery(
- op.getPartitionOptions(), op.getQuery(), Options.priority(rpcPriority));
+ ReadOperation op = c.element();
+
+ // While this creates a ServiceCallMetric for every input element, in reality, the number
+ // of input elements will either be very few (normally 1!), or they will differ and
+ // need different metrics.
+ ServiceCallMetric metric = ReadAll.buildServiceCallMetricForReadOp(config, op);
+
+ List<Partition> partitions;
+ try {
+ if (op.getQuery() != null) {
+ // Query was selected.
+ partitions =
+ batchTx.partitionQuery(
+ op.getPartitionOptions(),
+ op.getQuery(),
+ Options.priority(config.getRpcPriority().get()));
+ } else if (op.getIndex() != null) {
+ // Read with index was selected.
+ partitions =
+ batchTx.partitionReadUsingIndex(
+ op.getPartitionOptions(),
+ op.getTable(),
+ op.getIndex(),
+ op.getKeySet(),
+ op.getColumns(),
+ Options.priority(config.getRpcPriority().get()));
Review Comment:
Previously this was called without a priority if config.getRpcPriority() == null or config.getRpcPriority().get() == null.
Are we sure that this will not introduce a behavior change ?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/BatchSpannerRead.java:
##########
@@ -114,75 +132,62 @@ public void teardown() throws Exception {
@ProcessElement
public void processElement(ProcessContext c) throws Exception {
Transaction tx = c.sideInput(txView);
- BatchReadOnlyTransaction context =
+ BatchReadOnlyTransaction batchTx =
spannerAccessor.getBatchClient().batchReadOnlyTransaction(tx.transactionId());
- for (Partition p : execute(c.element(), context)) {
- c.output(p);
- }
- }
-
- private List<Partition> execute(ReadOperation op, BatchReadOnlyTransaction tx) {
- if (config.getRpcPriority() != null && config.getRpcPriority().get() != null) {
- return executeWithPriority(op, tx, config.getRpcPriority().get());
- } else {
- return executeWithoutPriority(op, tx);
- }
- }
-
- private List<Partition> executeWithoutPriority(ReadOperation op, BatchReadOnlyTransaction tx) {
- // Query was selected.
- if (op.getQuery() != null) {
- return tx.partitionQuery(op.getPartitionOptions(), op.getQuery());
- }
- // Read with index was selected.
- if (op.getIndex() != null) {
- return tx.partitionReadUsingIndex(
- op.getPartitionOptions(),
- op.getTable(),
- op.getIndex(),
- op.getKeySet(),
- op.getColumns());
- }
- // Read from table was selected.
- return tx.partitionRead(
- op.getPartitionOptions(), op.getTable(), op.getKeySet(), op.getColumns());
- }
-
- private List<Partition> executeWithPriority(
- ReadOperation op, BatchReadOnlyTransaction tx, RpcPriority rpcPriority) {
- // Query was selected.
- if (op.getQuery() != null) {
- return tx.partitionQuery(
- op.getPartitionOptions(), op.getQuery(), Options.priority(rpcPriority));
+ ReadOperation op = c.element();
+
+ // While this creates a ServiceCallMetric for every input element, in reality, the number
+ // of input elements will either be very few (normally 1!), or they will differ and
+ // need different metrics.
+ ServiceCallMetric metric = ReadAll.buildServiceCallMetricForReadOp(config, op);
+
+ List<Partition> partitions;
+ try {
+ if (op.getQuery() != null) {
+ // Query was selected.
+ partitions =
+ batchTx.partitionQuery(
+ op.getPartitionOptions(),
+ op.getQuery(),
+ Options.priority(config.getRpcPriority().get()));
+ } else if (op.getIndex() != null) {
+ // Read with index was selected.
+ partitions =
+ batchTx.partitionReadUsingIndex(
+ op.getPartitionOptions(),
+ op.getTable(),
+ op.getIndex(),
+ op.getKeySet(),
+ op.getColumns(),
+ Options.priority(config.getRpcPriority().get()));
+ } else {
+ // Read from table was selected.
+ partitions =
+ batchTx.partitionRead(
+ op.getPartitionOptions(),
+ op.getTable(),
+ op.getKeySet(),
+ op.getColumns(),
+ Options.priority(config.getRpcPriority().get()));
Review Comment:
Ditto.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/BatchSpannerRead.java:
##########
@@ -114,75 +132,62 @@ public void teardown() throws Exception {
@ProcessElement
public void processElement(ProcessContext c) throws Exception {
Transaction tx = c.sideInput(txView);
- BatchReadOnlyTransaction context =
+ BatchReadOnlyTransaction batchTx =
spannerAccessor.getBatchClient().batchReadOnlyTransaction(tx.transactionId());
- for (Partition p : execute(c.element(), context)) {
- c.output(p);
- }
- }
-
- private List<Partition> execute(ReadOperation op, BatchReadOnlyTransaction tx) {
- if (config.getRpcPriority() != null && config.getRpcPriority().get() != null) {
- return executeWithPriority(op, tx, config.getRpcPriority().get());
- } else {
- return executeWithoutPriority(op, tx);
- }
- }
-
- private List<Partition> executeWithoutPriority(ReadOperation op, BatchReadOnlyTransaction tx) {
- // Query was selected.
- if (op.getQuery() != null) {
- return tx.partitionQuery(op.getPartitionOptions(), op.getQuery());
- }
- // Read with index was selected.
- if (op.getIndex() != null) {
- return tx.partitionReadUsingIndex(
- op.getPartitionOptions(),
- op.getTable(),
- op.getIndex(),
- op.getKeySet(),
- op.getColumns());
- }
- // Read from table was selected.
- return tx.partitionRead(
- op.getPartitionOptions(), op.getTable(), op.getKeySet(), op.getColumns());
- }
-
- private List<Partition> executeWithPriority(
- ReadOperation op, BatchReadOnlyTransaction tx, RpcPriority rpcPriority) {
- // Query was selected.
- if (op.getQuery() != null) {
- return tx.partitionQuery(
- op.getPartitionOptions(), op.getQuery(), Options.priority(rpcPriority));
+ ReadOperation op = c.element();
+
+ // While this creates a ServiceCallMetric for every input element, in reality, the number
+ // of input elements will either be very few (normally 1!), or they will differ and
+ // need different metrics.
+ ServiceCallMetric metric = ReadAll.buildServiceCallMetricForReadOp(config, op);
+
+ List<Partition> partitions;
+ try {
Review Comment:
Is "executeWithoutPriority" path not needed anymore ?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java:
##########
@@ -377,6 +382,12 @@ public class SpannerIO {
// Multiple of mutation size to use to gather and sort mutations
private static final int DEFAULT_GROUPING_FACTOR = 1000;
+ // Size of caches for read/write ServiceCallMetric objects .
+ // This is a reasonable limit, as for reads, each worker will process very few different table
+ // read requests, and for writes, batching will ensure that write operations for the same
+ // table occur at
+ public static final int METRICS_CACHE_SIZE = 100;
Review Comment:
Can this be package private ?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1147537801
Note: Java PreCommit issues are unrelated to this PR.
This can now be merged
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1126494465
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1128918833
Ack. Thanks Niel.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1095205380
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on a diff in pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
nielm commented on code in PR #17335:
URL: https://github.com/apache/beam/pull/17335#discussion_r884734522
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/BatchSpannerRead.java:
##########
@@ -114,75 +132,62 @@ public void teardown() throws Exception {
@ProcessElement
public void processElement(ProcessContext c) throws Exception {
Transaction tx = c.sideInput(txView);
- BatchReadOnlyTransaction context =
+ BatchReadOnlyTransaction batchTx =
spannerAccessor.getBatchClient().batchReadOnlyTransaction(tx.transactionId());
- for (Partition p : execute(c.element(), context)) {
- c.output(p);
- }
- }
-
- private List<Partition> execute(ReadOperation op, BatchReadOnlyTransaction tx) {
- if (config.getRpcPriority() != null && config.getRpcPriority().get() != null) {
- return executeWithPriority(op, tx, config.getRpcPriority().get());
- } else {
- return executeWithoutPriority(op, tx);
- }
- }
-
- private List<Partition> executeWithoutPriority(ReadOperation op, BatchReadOnlyTransaction tx) {
- // Query was selected.
- if (op.getQuery() != null) {
- return tx.partitionQuery(op.getPartitionOptions(), op.getQuery());
- }
- // Read with index was selected.
- if (op.getIndex() != null) {
- return tx.partitionReadUsingIndex(
- op.getPartitionOptions(),
- op.getTable(),
- op.getIndex(),
- op.getKeySet(),
- op.getColumns());
- }
- // Read from table was selected.
- return tx.partitionRead(
- op.getPartitionOptions(), op.getTable(), op.getKeySet(), op.getColumns());
- }
-
- private List<Partition> executeWithPriority(
- ReadOperation op, BatchReadOnlyTransaction tx, RpcPriority rpcPriority) {
- // Query was selected.
- if (op.getQuery() != null) {
- return tx.partitionQuery(
- op.getPartitionOptions(), op.getQuery(), Options.priority(rpcPriority));
+ ReadOperation op = c.element();
+
+ // While this creates a ServiceCallMetric for every input element, in reality, the number
+ // of input elements will either be very few (normally 1!), or they will differ and
+ // need different metrics.
+ ServiceCallMetric metric = ReadAll.buildServiceCallMetricForReadOp(config, op);
+
+ List<Partition> partitions;
+ try {
+ if (op.getQuery() != null) {
+ // Query was selected.
+ partitions =
+ batchTx.partitionQuery(
+ op.getPartitionOptions(),
+ op.getQuery(),
+ Options.priority(config.getRpcPriority().get()));
+ } else if (op.getIndex() != null) {
+ // Read with index was selected.
+ partitions =
+ batchTx.partitionReadUsingIndex(
+ op.getPartitionOptions(),
+ op.getTable(),
+ op.getIndex(),
+ op.getKeySet(),
+ op.getColumns(),
+ Options.priority(config.getRpcPriority().get()));
Review Comment:
see above - config.getRpcPriority can no longer be null.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/BatchSpannerRead.java:
##########
@@ -114,75 +132,62 @@ public void teardown() throws Exception {
@ProcessElement
public void processElement(ProcessContext c) throws Exception {
Transaction tx = c.sideInput(txView);
- BatchReadOnlyTransaction context =
+ BatchReadOnlyTransaction batchTx =
spannerAccessor.getBatchClient().batchReadOnlyTransaction(tx.transactionId());
- for (Partition p : execute(c.element(), context)) {
- c.output(p);
- }
- }
-
- private List<Partition> execute(ReadOperation op, BatchReadOnlyTransaction tx) {
- if (config.getRpcPriority() != null && config.getRpcPriority().get() != null) {
- return executeWithPriority(op, tx, config.getRpcPriority().get());
- } else {
- return executeWithoutPriority(op, tx);
- }
- }
-
- private List<Partition> executeWithoutPriority(ReadOperation op, BatchReadOnlyTransaction tx) {
- // Query was selected.
- if (op.getQuery() != null) {
- return tx.partitionQuery(op.getPartitionOptions(), op.getQuery());
- }
- // Read with index was selected.
- if (op.getIndex() != null) {
- return tx.partitionReadUsingIndex(
- op.getPartitionOptions(),
- op.getTable(),
- op.getIndex(),
- op.getKeySet(),
- op.getColumns());
- }
- // Read from table was selected.
- return tx.partitionRead(
- op.getPartitionOptions(), op.getTable(), op.getKeySet(), op.getColumns());
- }
-
- private List<Partition> executeWithPriority(
- ReadOperation op, BatchReadOnlyTransaction tx, RpcPriority rpcPriority) {
- // Query was selected.
- if (op.getQuery() != null) {
- return tx.partitionQuery(
- op.getPartitionOptions(), op.getQuery(), Options.priority(rpcPriority));
+ ReadOperation op = c.element();
+
+ // While this creates a ServiceCallMetric for every input element, in reality, the number
+ // of input elements will either be very few (normally 1!), or they will differ and
+ // need different metrics.
+ ServiceCallMetric metric = ReadAll.buildServiceCallMetricForReadOp(config, op);
+
+ List<Partition> partitions;
+ try {
Review Comment:
Normally config.getRpcPriority() should never be null - a default is set in SpannerConfig.create().
However, I realise it is possible to pass null as a value (or as a null ValueProvider) to SpannerConfig.setRpcPriority().
This (IMHO) is incorrect. There is a Preconditions.checkNotNull above to ensure that getRpcPriority is non-null, and I have also added this same check to SpannerConfig.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1095205393
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1095205377
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1095205382
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #17335: [BEAM-14121] Fix SpannerIO service call metrics and improve tests.
Posted by GitBox <gi...@apache.org>.
aaltay commented on PR #17335:
URL: https://github.com/apache/beam/pull/17335#issuecomment-1105664056
R: @chamikaramj
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org