You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "reuvenlax (via GitHub)" <gi...@apache.org> on 2023/08/17 17:34:11 UTC
[GitHub] [beam] reuvenlax opened a new pull request, #28050: Don't invalidate streams on quota errors
reuvenlax opened a new pull request, #28050:
URL: https://github.com/apache/beam/pull/28050
R: @scwhittle
--
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
Re: [PR] Don't invalidate streams on quota errors [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1791453936
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] scwhittle commented on pull request #28050: Don't invalidate streams on quota errors
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1738697286
waiting on author
--
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] github-actions[bot] commented on pull request #28050: Don't invalidate streams on quota errors
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1711575547
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @Abacn for label java.
R: @Abacn for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
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
Re: [PR] Don't invalidate streams on quota errors [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1783557714
> @liferoad saw that you added the 2.52.0 milestone - is this actually a release blocker or can we remove the milestone?
>
> cc/ @reuvenlax @scwhittle
We got couple of customer issues that are related to this. If we could get this done for 2.52.0, that will be great.
--
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] github-actions[bot] commented on pull request #28050: Don't invalidate streams on quota errors
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1682775052
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @kennknowles for label java.
R: @bvolpato for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review 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] github-actions[bot] commented on pull request #28050: Don't invalidate streams on quota errors
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1706508192
Reminder, please take a look at this pr: @kennknowles @bvolpato
--
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
Re: [PR] Don't invalidate streams on quota errors [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1789090012
Moving to 2.53 since this is not a 2.52 regression and this seems unlikely to make the cut
--
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] github-actions[bot] commented on pull request #28050: Don't invalidate streams on quota errors
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1727603934
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @kennknowles for label java.
R: @johnjcasey for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
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
Re: [PR] Don't invalidate streams on quota errors [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on code in PR #28050:
URL: https://github.com/apache/beam/pull/28050#discussion_r1379710182
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileIO.java:
##########
@@ -227,7 +227,7 @@
* {@link Sink}, e.g. write different elements to Avro files in different directories with different
* schemas.
*
- * <p>This feature is supported by {@link #writeDynamic}. Use {@link Write#by} to specify how to
+ * <p>This feature is supported by {@link #writeDynamic}. Use {@link Write#by} to specify how too
Review Comment:
revert
--
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] scwhittle commented on a diff in pull request #28050: Don't invalidate streams on quota errors
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on code in PR #28050:
URL: https://github.com/apache/beam/pull/28050#discussion_r1297697681
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWriteUnshardedRecords.java:
##########
@@ -663,7 +663,16 @@ long flush(
retrieveErrorDetails(contexts));
failedContext.failureCount += 1;
- invalidateWriteStream();
+ Throwable error = Preconditions.checkStateNotNull(failedContext.getError());
Review Comment:
Line 607 and retrieveErrorDetails seems to expect this is null sometimes, perhaps safer to just set quotaError to false if getError is null?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWriteUnshardedRecords.java:
##########
@@ -663,7 +663,16 @@ long flush(
retrieveErrorDetails(contexts));
failedContext.failureCount += 1;
- invalidateWriteStream();
+ Throwable error = Preconditions.checkStateNotNull(failedContext.getError());
+ Status.Code statusCode = Status.fromThrowable(error).getCode();
+ boolean quotaError = statusCode.equals(Status.Code.RESOURCE_EXHAUSTED);
+
+ if (!quotaError) {
+ // This forces us to close and reopen all gRPC connections to Storage API on error,
+ // which empirically
+ // fixes random stuckness issues.
+ invalidateWriteStream();
Review Comment:
invalidateWriteStream is currently just unpinning. Should it close() as well?
In the recent issue we appeared to have leaks of ManagedChannels in AppendClient, could that have been because they were not closed there?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWriteUnshardedRecords.java:
##########
@@ -663,7 +663,16 @@ long flush(
retrieveErrorDetails(contexts));
failedContext.failureCount += 1;
- invalidateWriteStream();
+ Throwable error = Preconditions.checkStateNotNull(failedContext.getError());
+ Status.Code statusCode = Status.fromThrowable(error).getCode();
+ boolean quotaError = statusCode.equals(Status.Code.RESOURCE_EXHAUSTED);
+
+ if (!quotaError) {
+ // This forces us to close and reopen all gRPC connections to Storage API on error,
+ // which empirically
+ // fixes random stuckness issues.
Review Comment:
nit: weird wrapping
--
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] scwhittle commented on pull request #28050: Don't invalidate streams on quota errors
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1682923080
Would also be good to add some tests (especially if the missing close is a real problem)
--
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
Re: [PR] Don't invalidate streams on quota errors [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on code in PR #28050:
URL: https://github.com/apache/beam/pull/28050#discussion_r1379486984
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWriteUnshardedRecords.java:
##########
@@ -663,7 +663,16 @@ long flush(
retrieveErrorDetails(contexts));
failedContext.failureCount += 1;
- invalidateWriteStream();
+ Throwable error = Preconditions.checkStateNotNull(failedContext.getError());
+ Status.Code statusCode = Status.fromThrowable(error).getCode();
+ boolean quotaError = statusCode.equals(Status.Code.RESOURCE_EXHAUSTED);
+
+ if (!quotaError) {
+ // This forces us to close and reopen all gRPC connections to Storage API on error,
+ // which empirically
+ // fixes random stuckness issues.
Review Comment:
done
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWriteUnshardedRecords.java:
##########
@@ -663,7 +663,16 @@ long flush(
retrieveErrorDetails(contexts));
failedContext.failureCount += 1;
- invalidateWriteStream();
+ Throwable error = Preconditions.checkStateNotNull(failedContext.getError());
Review Comment:
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
Re: [PR] Don't invalidate streams on quota errors [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax merged PR #28050:
URL: https://github.com/apache/beam/pull/28050
--
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] bvolpato commented on a diff in pull request #28050: Don't invalidate streams on quota errors
Posted by "bvolpato (via GitHub)" <gi...@apache.org>.
bvolpato commented on code in PR #28050:
URL: https://github.com/apache/beam/pull/28050#discussion_r1297705310
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWritesShardedRecords.java:
##########
@@ -664,21 +664,28 @@ public void process(
boolean offsetMismatch =
statusCode.equals(Code.OUT_OF_RANGE) || statusCode.equals(Code.ALREADY_EXISTS);
- // Invalidate the StreamWriter and force a new one to be created.
+ boolean quotaError = statusCode.equals(Code.RESOURCE_EXHAUSTED);
if (!offsetMismatch) {
// Don't log errors for expected offset mismatch. These will be logged as warnings
// below.
LOG.error(
"Got error " + failedContext.getError() + " closing " + failedContext.streamName);
}
- // TODO: Only do this on explicit NOT_FOUND errors once BigQuery reliably produces them.
try {
+ // TODO: Only do this on explicit NOT_FOUND errors once BigQuery reliably produces
+ // them.
tryCreateTable.call();
} catch (Exception e) {
throw new RuntimeException(e);
}
- clearClients.accept(failedContexts);
+
+ if (!quotaError) {
+ // This forces us to close and reopen all gRPC connections to Storage API on error,
+ // which empirically
+ // fixes random stuckness issues.
+ clearClients.accept(failedContexts);
+ }
Review Comment:
What should we do in case the error is `RESOURCE_EXHAUSTED`, but the resource in question is connections? (`Exceeds 'Concurrent connections' quota`)
Are those clients healthy / can be reused, or not? I would assume that they aren't, but I don't know how the next usage would behave.
It would be great to have a server-side signal from BigQuery client on what is recoverable or not, but I believe we don't have anything like that today.
--
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] github-actions[bot] commented on pull request #28050: Don't invalidate streams on quota errors
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1693261657
Reminder, please take a look at this pr: @kennknowles @bvolpato
--
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 #28050: Don't invalidate streams on quota errors
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1696433612
Any updates ?
--
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
Re: [PR] Don't invalidate streams on quota errors [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1783376655
@liferoad saw that you added the 2.52.0 milestone - is this actually a release blocker or can we remove the milestone?
cc/ @reuvenlax @scwhittle
--
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
Re: [PR] Don't invalidate streams on quota errors [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1787786837
@reuvenlax what are next steps here? The release is supposed to be cut tomorrow, is it realistic to get this in?
--
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
Re: [PR] Don't invalidate streams on quota errors [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on code in PR #28050:
URL: https://github.com/apache/beam/pull/28050#discussion_r1379485049
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWritesShardedRecords.java:
##########
@@ -664,21 +664,28 @@ public void process(
boolean offsetMismatch =
statusCode.equals(Code.OUT_OF_RANGE) || statusCode.equals(Code.ALREADY_EXISTS);
- // Invalidate the StreamWriter and force a new one to be created.
+ boolean quotaError = statusCode.equals(Code.RESOURCE_EXHAUSTED);
if (!offsetMismatch) {
// Don't log errors for expected offset mismatch. These will be logged as warnings
// below.
LOG.error(
"Got error " + failedContext.getError() + " closing " + failedContext.streamName);
}
- // TODO: Only do this on explicit NOT_FOUND errors once BigQuery reliably produces them.
try {
+ // TODO: Only do this on explicit NOT_FOUND errors once BigQuery reliably produces
+ // them.
tryCreateTable.call();
} catch (Exception e) {
throw new RuntimeException(e);
}
- clearClients.accept(failedContexts);
+
+ if (!quotaError) {
+ // This forces us to close and reopen all gRPC connections to Storage API on error,
+ // which empirically
+ // fixes random stuckness issues.
+ clearClients.accept(failedContexts);
+ }
Review Comment:
I don't think that should happen on append. That sounds like an error that would happen when opening a stream
--
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] reuvenlax commented on a diff in pull request #28050: Don't invalidate streams on quota errors
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on code in PR #28050:
URL: https://github.com/apache/beam/pull/28050#discussion_r1297726746
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWriteUnshardedRecords.java:
##########
@@ -663,7 +663,16 @@ long flush(
retrieveErrorDetails(contexts));
failedContext.failureCount += 1;
- invalidateWriteStream();
+ Throwable error = Preconditions.checkStateNotNull(failedContext.getError());
+ Status.Code statusCode = Status.fromThrowable(error).getCode();
+ boolean quotaError = statusCode.equals(Status.Code.RESOURCE_EXHAUSTED);
+
+ if (!quotaError) {
+ // This forces us to close and reopen all gRPC connections to Storage API on error,
+ // which empirically
+ // fixes random stuckness issues.
+ invalidateWriteStream();
Review Comment:
close() is called on cache eviction, and invalidateWriteStream also evicts from the cache. Cache owns the object, so we only want to close on cache eviction.
--
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] github-actions[bot] commented on pull request #28050: Don't invalidate streams on quota errors
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1737274147
Reminder, please take a look at this pr: @kennknowles @johnjcasey
--
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] github-actions[bot] commented on pull request #28050: Don't invalidate streams on quota errors
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28050:
URL: https://github.com/apache/beam/pull/28050#issuecomment-1722217701
Reminder, please take a look at this pr: @Abacn @Abacn
--
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