You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "ahmedabu98 (via GitHub)" <gi...@apache.org> on 2024/07/02 17:04:08 UTC
[PR] Add options to control number of Storage API connections when using multiplexing [beam]
ahmedabu98 opened a new pull request, #31721:
URL: https://github.com/apache/beam/pull/31721
Controls min and max number of connections to connection pool. For more details on this BigQuery Storage Write API feature, see https://cloud.google.com/bigquery/docs/write-api-best-practices#connection_pool_management
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1665874890
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
+
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the maximum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "This value should be greater than or equal to the total number of dynamic destinations, otherwise a "
+ + "race condition occurs where append operations compete over streams.")
+ @Default.Integer(20)
Review Comment:
> Where does 20 come from?
These values are copied over from the existing ConnectionWorkerPool defaults (see [code ref](https://github.com/googleapis/java-bigquerystorage/blob/b255926764e0e9dac689efdd31d38a25b251296f/google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorkerPool.java#L175-L191)). Note that this restriction is only applied when a user explicitly enables multiplexing (ie. setUseStorageApiConnectionPool(true) )
> could we be more explicit about the implications of the race condition?
Good call out, will make it more clear. This just affects performance. Individual append operations may fail when no streams are available, but we retry enough for records to ultimately make it to the table .
> Can we detect when this race condition might occur
StreamWriter (the BQ class doing append operations) will log why an append fails. In this case, we get a `PERMISSION_DENIED: Permission 'TABLES_UPDATE_DATA' denied on resource` error. Example below:
```
Connection finished with error com.google.api.gax.rpc.PermissionDeniedException:
io.grpc.StatusRuntimeException: PERMISSION_DENIED: Permission 'TABLES_UPDATE_DATA' denied on resource
'projects/google.com:clouddfe/datasets/ahmedabualsaud/tables/a2batch_instants_1' (or it may not exist).
for stream projects/google.com:clouddfe/datasets/ahmedabualsaud/tables/a2batch_instants_4/streams/_default
with write id: 1f52a5a0-c1a3-47ee-8f44-73b7f06a8aa9
```
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #31721:
URL: https://github.com/apache/beam/pull/31721#issuecomment-2197767706
R: @gaolemeng
R: @agrawal-siddharth
CC: @reuvenlax
CC: @damccorm
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1663730726
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
+
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the maximum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "This value should be greater than or equal to the total number of dynamic destinations, otherwise a "
+ + "race condition occurs where append operations compete over streams.")
+ @Default.Integer(20)
Review Comment:
A few questions here:
1) Where does 20 come from? I'm noting that from https://cloud.google.com/bigquery/docs/write-api-best-practices#connection_pool_management 20 connections seems more like a floor than a ceiling? This has the potential to degrade existing jobs which write to >20 destinations, so we should be careful here.
2) "otherwise a race condition occurs where append operations compete over streams." - could we be more explicit about the implications of the race condition? Its degraded performance, not correctness, right?
3) Can we detect when this race condition might occur and warn? I know we can't ahead of time, but do we get any warnings or anything like that back? Alternately, can we track the number of dynamic destinations we're writing to? (not sure if there's a clean path to doing this)
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
Review Comment:
Is there value in exposing this to a customer? When would they want to tune this?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -122,6 +142,9 @@ public interface BigQueryOptions
void setStorageWriteMaxInflightBytes(Long value);
+ @Description(
+ "Enables multiplexing mode, where multiple tables can share the same connection. Only available when writing with STORAGE_API_AT_LEAST_ONCE"
+ + " mode. For more information, see https://cloud.google.com/bigquery/docs/write-api-best-practices#connection_pool_management")
Review Comment:
Could we add that
1) this is recommended when writing to >20 tables - I think this is true based on the linked doc, if not maybe we can add other guidance on when to do this
2) Can we add something like "when using multiplexing mode, consider limiting `MaxConnectionPoolConnections` as well"?
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
+
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the maximum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "This value should be greater than or equal to the total number of dynamic destinations, otherwise a "
+ + "race condition occurs where append operations compete over streams.")
+ @Default.Integer(20)
Review Comment:
> Where does 20 come from?
More specifically - can we make this significantly higher without running into quota issues? How many connections are jobs that are running into problems using? IIRC it is orders of magnitude higher than this
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1672005611
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
+
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the maximum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "This value should be greater than or equal to the total number of dynamic destinations, otherwise a "
+ + "race condition occurs where append operations compete over streams.")
+ @Default.Integer(20)
Review Comment:
> These values are copied over from the existing ConnectionWorkerPool defaults (see [code ref](https://github.com/googleapis/java-bigquerystorage/blob/b255926764e0e9dac689efdd31d38a25b251296f/google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorkerPool.java#L175-L191)). Note that this restriction is only applied when a user explicitly enables multiplexing (ie. setUseStorageApiConnectionPool(true) )
Got it - that is reasonable/non-blocking for this PR then. I do wonder if making this larger makes sense though, I'd be interested in hearing thoughts from some of the BQ folks here.
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1665874890
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
+
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the maximum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "This value should be greater than or equal to the total number of dynamic destinations, otherwise a "
+ + "race condition occurs where append operations compete over streams.")
+ @Default.Integer(20)
Review Comment:
> Where does 20 come from?
These values are copied over from the existing ConnectionWorkerPool defaults (see [code ref](https://github.com/googleapis/java-bigquerystorage/blob/b255926764e0e9dac689efdd31d38a25b251296f/google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorkerPool.java#L175-L191)). Note that this restriction is only applied when a user explicitly enables multiplexing (ie. set setUseStorageApiConnectionPool
> could we be more explicit about the implications of the race condition?
Good call out, will make it more clear. This just affects performance. Individual append operations may fail when no streams are available, but we retry enough for records to ultimately make it to the table .
> Can we detect when this race condition might occur
StreamWriter (the BQ class doing append operations) will log why an append fails. In this case, we get a `PERMISSION_DENIED: Permission 'TABLES_UPDATE_DATA' denied on resource` error. Example below:
```
Connection finished with error com.google.api.gax.rpc.PermissionDeniedException:
io.grpc.StatusRuntimeException: PERMISSION_DENIED: Permission 'TABLES_UPDATE_DATA' denied on resource
'projects/google.com:clouddfe/datasets/ahmedabualsaud/tables/a2batch_instants_1' (or it may not exist).
for stream projects/google.com:clouddfe/datasets/ahmedabualsaud/tables/a2batch_instants_4/streams/_default
with write id: 1f52a5a0-c1a3-47ee-8f44-73b7f06a8aa9
```
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "agrawal-siddharth (via GitHub)" <gi...@apache.org>.
agrawal-siddharth commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1674624045
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,29 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations). BigQuery will create this many connections at first "
Review Comment:
Existing text looks fine. I was just clarifying behavior.
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1665884041
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
Review Comment:
The original request was actually for this option. BigQuery fixes this to 2 connections per worker and only creates new connections when those two are "overwhelmed" (I didn't dig into how this is calculated).
The request is to allow tuning in the hopes that increasing min num connections will improve performance
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1674381405
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,29 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
Review Comment:
By worker I mean machine. Within a worker, each thread/vCPU/DoFn creates a StreamWriter. I'm guessing each worker has its own map that its threads can access?
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "agrawal-siddharth (via GitHub)" <gi...@apache.org>.
agrawal-siddharth commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1674575842
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,29 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
Review Comment:
OK, then yes that is correct. I suggest adding the "before any connections are shared" blurb.
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1665876385
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
+
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the maximum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "This value should be greater than or equal to the total number of dynamic destinations, otherwise a "
+ + "race condition occurs where append operations compete over streams.")
+ @Default.Integer(20)
Review Comment:
> How many connections are jobs that are running into problems using? IIRC it is orders of magnitude higher than this
I think the issue you're referencing is a different one where STORAGE_WRITE_API batch would hit concurrent connections quota. It's been fixed by #31710
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1665912389
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -122,6 +142,9 @@ public interface BigQueryOptions
void setStorageWriteMaxInflightBytes(Long value);
+ @Description(
+ "Enables multiplexing mode, where multiple tables can share the same connection. Only available when writing with STORAGE_API_AT_LEAST_ONCE"
+ + " mode. For more information, see https://cloud.google.com/bigquery/docs/write-api-best-practices#connection_pool_management")
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1674387813
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,29 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations). BigQuery will create this many connections at first "
Review Comment:
Is there anything I should change here?
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #31721:
URL: https://github.com/apache/beam/pull/31721#issuecomment-2226175447
Thanks for taking a look @agrawal-siddharth! Merging now
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1671281372
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -122,6 +145,11 @@ public interface BigQueryOptions
void setStorageWriteMaxInflightBytes(Long value);
+ @Description(
+ "Enables multiplexing mode, where multiple tables can share the same connection. Only available when writing with STORAGE_API_AT_LEAST_ONCE"
+ + " mode. This is recommended if your write operation is creating 20+ connections. When using multiplexing, consider tuning "
+ + "the number of connections created by the connection pool with minConnectionPoolConnections and maxConnectionPoolConnections. "
+ + "For more information, see https://cloud.google.com/bigquery/docs/write-api-best-practices#connection_pool_management")
@Default.Boolean(false)
Review Comment:
I wonder if it's time to enable this by default (though we should probably update our BOM to point to a more-recent client lib first).
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1665884555
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
Review Comment:
Will add a recommendation to increase this if running into performance issues
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1674381405
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,29 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
Review Comment:
By worker I mean machine. Within a worker, each thread/vCPU/DoFn creates a StreamWriter. So I'm guessing each worker has its own map that its threads can access
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1674637805
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,29 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1674393832
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
+
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the maximum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "This value should be greater than or equal to the total number of dynamic destinations, otherwise a "
+ + "race condition occurs where append operations compete over streams.")
+ @Default.Integer(20)
Review Comment:
Sounds good, will change the tone to not encourage it so strongly
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1672016838
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -122,6 +145,11 @@ public interface BigQueryOptions
void setStorageWriteMaxInflightBytes(Long value);
+ @Description(
+ "Enables multiplexing mode, where multiple tables can share the same connection. Only available when writing with STORAGE_API_AT_LEAST_ONCE"
+ + " mode. This is recommended if your write operation is creating 20+ connections. When using multiplexing, consider tuning "
+ + "the number of connections created by the connection pool with minConnectionPoolConnections and maxConnectionPoolConnections. "
+ + "For more information, see https://cloud.google.com/bigquery/docs/write-api-best-practices#connection_pool_management")
@Default.Boolean(false)
Review Comment:
+1 to trying this, I'd probably vote to to do it separately after the release cut though to give it time to bake and so we can check on benchmarks.
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1672624495
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -122,6 +145,11 @@ public interface BigQueryOptions
void setStorageWriteMaxInflightBytes(Long value);
+ @Description(
+ "Enables multiplexing mode, where multiple tables can share the same connection. Only available when writing with STORAGE_API_AT_LEAST_ONCE"
+ + " mode. This is recommended if your write operation is creating 20+ connections. When using multiplexing, consider tuning "
+ + "the number of connections created by the connection pool with minConnectionPoolConnections and maxConnectionPoolConnections. "
+ + "For more information, see https://cloud.google.com/bigquery/docs/write-api-best-practices#connection_pool_management")
@Default.Boolean(false)
Review Comment:
Enabling this as a default may have negative effects on pipelines writing to >20 destinations (since maxConnectionsPerRegion is 20 by default).
Whereas currently the sink would create 1 connection per destination, enabling multiplexing would limit it to 20 connections (unless the user explicitly sets a higher maximum)
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 merged PR #31721:
URL: https://github.com/apache/beam/pull/31721
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "agrawal-siddharth (via GitHub)" <gi...@apache.org>.
agrawal-siddharth commented on code in PR #31721:
URL: https://github.com/apache/beam/pull/31721#discussion_r1672915640
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,29 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
Review Comment:
"this option sets the minimum number of connections each pool creates before any connections are shared."
When you say "per worker" I'm not sure what a worker corresponds to within the writeAPI storage library. Does each worker create its own StreamWriter? Under the hood, the connection pool is a static map whose KEY is the region, and the VALUE is the pool for that region. So all StreamWriter objects within the same process will share the same map. This could span many workers if they run within the same process.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,26 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations).")
+ @Default.Integer(2)
+ Integer getMinConnectionPoolConnections();
+
+ void setMinConnectionPoolConnections(Integer value);
+
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the maximum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "This value should be greater than or equal to the total number of dynamic destinations, otherwise a "
+ + "race condition occurs where append operations compete over streams.")
+ @Default.Integer(20)
Review Comment:
We have 2 general concerns with too many connections:
(a) the user will hit the concurrent connection limit, and
(b) it is not fruitful to further increase the number of connections because a single machine can only send so much traffic over the network. From experiments going beyond about 20 connections does not help with increasing throughput.
Due to the above, we don't expect it will help much to raise this limit. So I think a customer is less likely to want to tune this parameter but we have it for completeness.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryOptions.java:
##########
@@ -109,6 +109,29 @@ public interface BigQueryOptions
void setNumStorageWriteApiStreamAppendClients(Integer value);
+ @Description(
+ "When using the STORAGE_API_AT_LEAST_ONCE write method with multiplexing (ie. useStorageApiConnectionPool=true), "
+ + "this option sets the minimum number of connections each pool creates. This is on a per worker, per region basis. "
+ + "Note that in practice, the minimum number of connections created is the minimum of this value and "
+ + "(numStorageWriteApiStreamAppendClients x num destinations). BigQuery will create this many connections at first "
Review Comment:
This is the minimum number of connections per region. Note that BigQuery creates the connections on demand. Up to the minimum, there is no connection sharing. Once the minimum has been reached, existing connections will be shared until they are saturated. Once all existing connections are saturated then new connections, up to the maximum, will be created.
--
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] Add options to control number of Storage API connections when using multiplexing [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31721:
URL: https://github.com/apache/beam/pull/31721#issuecomment-2197768239
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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