fix: Wiring `bigQueryThreadPoolTaskScheduler` with `writeJsonStream` #1855

prash-mi · 2023-05-16T10:15:39Z

This PR Wires ThreadPoolTaskScheduler with writeJsonStream.

User can use bigQueryThreadPoolTaskScheduler in order to avoid java.lang.OutOfMemoryError which was arising due to too many concurrent write threads working.

Fixes #1599

burkedavison · 2023-05-19T14:32:34Z

...cloud-gcp-bigquery/src/main/java/com/google/cloud/spring/bigquery/core/BigQueryTemplate.java

@@ -115,6 +120,9 @@ public BigQueryTemplate(
    this.datasetName = bqDatasetName;
    this.taskScheduler = taskScheduler;
    this.bigQueryWriteClient = bigQueryWriteClient;
+    ThreadPoolTaskScheduler threadPoolTaskScheduler = (ThreadPoolTaskScheduler) taskScheduler;


Unchecked cast from TaskScheduler to ThreadPoolTaskScheduler.

Changed it to ExecutorService instead

...cloud-gcp-bigquery/src/main/java/com/google/cloud/spring/bigquery/core/BigQueryTemplate.java

...-cloud-gcp-bigquery/src/test/java/com/google/cloud/spring/bigquery/BigQueryTemplateTest.java

meltsufin · 2023-05-23T01:02:56Z

...cloud-gcp-bigquery/src/main/java/com/google/cloud/spring/bigquery/core/BigQueryTemplate.java

@@ -86,6 +90,7 @@ public class BigQueryTemplate implements BigQueryOperations {
  private final Logger logger = LoggerFactory.getLogger(BigQueryTemplate.class);

  private final int jsonWriterBatchSize;
+  private final ScheduledExecutorService executorService;


Why not just ExecutorService? You're not using the scheduling features.
Also, the variable should probably be called jsonWriterExecutorService for clarity.

Makes sense, wired a ExecutorService

meltsufin · 2023-05-23T01:05:10Z

...cloud-gcp-bigquery/src/main/java/com/google/cloud/spring/bigquery/core/BigQueryTemplate.java

@@ -115,6 +120,9 @@ public BigQueryTemplate(
    this.datasetName = bqDatasetName;
    this.taskScheduler = taskScheduler;
    this.bigQueryWriteClient = bigQueryWriteClient;
+    ThreadPoolTaskScheduler threadPoolTaskScheduler = (ThreadPoolTaskScheduler) taskScheduler;


taskScheduler is "used to poll for the status of long-running BigQuery operations". I don't think it should be re-purposed for json writing as well.
Instead, I would add a setter for jsonWriterExecutorService and wire in a default one from BigQueryAutoConfiguration.

sure, that's a better approach, I have updated the implementation accordingly, thanks

meltsufin · 2023-05-25T17:39:30Z

...cloud-gcp-bigquery/src/main/java/com/google/cloud/spring/bigquery/core/BigQueryTemplate.java

-            // This can be replaced with interrupting the ExecutorService when it has been wired-in
-            logger.info("asyncTask interrupted");
+          if (exception != null || !writeApiResponse.isSuccessful()) {
+            logger.error("asyncTask interrupted");


Does "asyncTask interrupted" still make sense? Why not just log the exception and/or the write response error message?

sure, exception might be null so logging it when it's not null, and appending Write operation failed warning otherwise

meltsufin · 2023-05-25T17:45:29Z

...cloud-gcp-bigquery/src/main/java/com/google/cloud/spring/bigquery/core/BigQueryTemplate.java

   */
  public BigQueryTemplate(
      BigQuery bigQuery,
      BigQueryWriteClient bigQueryWriteClient,
      Map<String, Object> bqInitSettings,
-      TaskScheduler taskScheduler) {
+      TaskScheduler taskScheduler,
+      ExecutorService jsonWriterExecutorService) {


This is a breaking change. Either add a setter or an overloaded constructor.

My bad, I thought that we have updated the bigQueryTemplate bean @ GcpBigQueryAutoConfiguration , so we are safe from this breaking change! But you are right and a user might directly initialise BigQueryTemplate.

So, I have added an overloaded constructor where the user need not pass jsonWriterExecutorService (it initialises a default FixedThreadPool)

meltsufin

@prash-mi Can we try re-using the bigQueryThreadPoolTaskScheduler instead of introducing a new ExecutorService specific to jsonWriter? I think it will be simpler and more clear.

prash-mi · 2023-06-09T05:29:54Z

@prash-mi Can we try re-using the bigQueryThreadPoolTaskScheduler instead of introducing a new ExecutorService specific to jsonWriter? I think it will be simpler and more clear.

@meltsufin sure, I have reused bigQueryThreadPoolTaskScheduler PTAL.
Currently I have left jsonWriterExecutorService as is in the code as later we might be able to use it with any new capabilities we implement (with supporting documentation). Please let me know if it looks good, or if we should remove it from all the layers (GcpBigQueryAutoConfiguration and Unit Tests)

meltsufin

I was looking to have the jsonWriterExecutorService and everything associated with it to be removed entirely. Do you see a problem with that?

prash-mi · 2023-06-15T05:52:43Z

I was looking to have the jsonWriterExecutorService and everything associated with it to be removed entirely. Do you see a problem with that?

Done @meltsufin, I have removed jsonWriterExecutorService from all the layers

meltsufin

Just final polish comments.

meltsufin · 2023-06-15T14:34:26Z

...c/main/java/com/google/cloud/spring/autoconfigure/bigquery/GcpBigQueryAutoConfiguration.java

    Map<String, Object> bqInitSettings = new HashMap<>();
    bqInitSettings.put("DATASET_NAME", this.datasetName);
    bqInitSettings.put("JSON_WRITER_BATCH_SIZE", this.jsonWriterBatchSize);
+    bqInitSettings.put("JSON_WRITER_THREAD_POOL_SIZE", this.threadPoolSize);


Is this still needed?

No, it was used with the previous ExecutorService. Removed it

meltsufin · 2023-06-15T14:34:46Z

...cloud-gcp-bigquery/src/main/java/com/google/cloud/spring/bigquery/core/BigQueryTemplate.java

@@ -81,6 +83,9 @@ public class BigQueryTemplate implements BigQueryOperations {
  private static final int DEFAULT_JSON_STREAM_WRITER_BATCH_SIZE =
      1000; // write records in batches of 1000

+  private static final int DEFAULT_JSON_WRITER_THREAD_POOL_SIZE =


Seems unused.

...cloud-gcp-bigquery/src/main/java/com/google/cloud/spring/bigquery/core/BigQueryTemplate.java

sonarcloud · 2023-06-16T06:01:07Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

82.4% Coverage
0.0% Duplication

...cloud-gcp-bigquery/src/main/java/com/google/cloud/spring/bigquery/core/BigQueryTemplate.java

meltsufin · 2023-06-16T13:37:36Z

...cloud-gcp-bigquery/src/main/java/com/google/cloud/spring/bigquery/core/BigQueryTemplate.java

+              asyncTaskScheduledFuture.cancel(true);
+            }
+          } else if (writeApiResponse != null && !writeApiResponse.isSuccessful()) {
+            logger.warn("Write operation failed");


Would it make sense to log a message from the writeApiResponse here?

It might be tricky to log it here as WriteApiResponse returns a List<StorageError> using writeApiResponse.getErrors() (as it's a batch operation), users have access to writeApiResponse, so they might need to process it.

prash-mi added 5 commits May 16, 2023 15:00

Wired executorService

e9c8cdb

update ThreadPoolTaskScheduler

de950e4

updated writeJsonStream

4f76c48

add writeJsonStreamNegativeTest and writeJsonStreamThrowTest

ac88648

lint

04bf7e5

prash-mi marked this pull request as ready for review May 19, 2023 13:59

prash-mi requested review from meltsufin and burkedavison May 19, 2023 14:01

burkedavison reviewed May 19, 2023

View reviewed changes

meltsufin requested changes May 23, 2023

View reviewed changes

prash-mi added 6 commits May 25, 2023 11:48

add jsonWriterExecutorService bean

d7da087

wired in jsonWriterExecutorService

ffa7230

wired getDefaultExecutor

c180bf1

wired jsonWriterExecutorService

7153d52

updated writeJsonStreamNegativeTest and writeJsonStreamThrowTest

13dc204

updated writejsonSteam method

3221051

meltsufin requested changes May 25, 2023

View reviewed changes

prash-mi added 9 commits May 26, 2023 12:41

add writeJsonStreamTestDefaultPool

44bbc66

add BigQueryTemplate overloaded constructor

bc5f0df

added more logging at writeJsonStream

a3de279

sonar

2907621

updated bqTemplateDefaultPool

15c3f93

updated jsonWriterExecutorService init

b3329fc

updated setup

e4ad7f1

add JSON_WRITER_THREAD_POOL_SIZE

63c53e6

Minor refactor

116d313

meltsufin requested changes May 31, 2023

View reviewed changes

Reuse TaskScheduler with writeJsonStream

9ff016e

prash-mi requested review from a team as code owners June 9, 2023 05:22

meltsufin requested changes Jun 14, 2023

View reviewed changes

prash-mi added 4 commits June 15, 2023 11:18

removed jsonWriterExecutorService

334e7ff

removed overloaded and jsonWriterExecutorService

4a0e8ac

updated BigQueryTemplate constructor

2420747

updated bigQueryTemplate bean

1a22971

prash-mi changed the title ~~fix: Wiring ExecutorService with writeJsonStream~~ fix: Wiring bigQueryThreadPoolTaskScheduler with writeJsonStream Jun 15, 2023

meltsufin requested changes Jun 15, 2023

View reviewed changes

prash-mi added 2 commits June 16, 2023 10:58

removed JSON_WRITER_THREAD_POOL_SIZE property

aa70ed2

refactor

db08f69

meltsufin approved these changes Jun 16, 2023

View reviewed changes

meltsufin merged commit 6467a08 into GoogleCloudPlatform:main Jun 20, 2023

release-please bot mentioned this pull request Jun 20, 2023

chore(main): release 4.5.1 #1972

Merged

release-please bot mentioned this pull request Dec 18, 2023

chore(main): release 5.0.1 #2467

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Wiring `bigQueryThreadPoolTaskScheduler` with `writeJsonStream` #1855

fix: Wiring `bigQueryThreadPoolTaskScheduler` with `writeJsonStream` #1855

prash-mi commented May 16, 2023 •

edited

Loading

burkedavison May 19, 2023

prash-mi May 25, 2023

meltsufin May 23, 2023

prash-mi May 25, 2023

meltsufin May 23, 2023

prash-mi May 25, 2023

meltsufin May 25, 2023

prash-mi May 26, 2023

meltsufin May 25, 2023

prash-mi May 26, 2023 •

edited

Loading

meltsufin left a comment

prash-mi commented Jun 9, 2023

meltsufin left a comment

prash-mi commented Jun 15, 2023

meltsufin left a comment

meltsufin Jun 15, 2023

prash-mi Jun 16, 2023

meltsufin Jun 15, 2023

prash-mi Jun 16, 2023

sonarcloud bot commented Jun 16, 2023

meltsufin Jun 16, 2023

prash-mi Jun 19, 2023

fix: Wiring bigQueryThreadPoolTaskScheduler with writeJsonStream #1855

fix: Wiring bigQueryThreadPoolTaskScheduler with writeJsonStream #1855

Conversation

prash-mi commented May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prash-mi May 26, 2023 • edited Loading

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

prash-mi commented Jun 9, 2023

meltsufin left a comment

Choose a reason for hiding this comment

prash-mi commented Jun 15, 2023

meltsufin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix: Wiring `bigQueryThreadPoolTaskScheduler` with `writeJsonStream` #1855

fix: Wiring `bigQueryThreadPoolTaskScheduler` with `writeJsonStream` #1855

prash-mi commented May 16, 2023 •

edited

Loading

prash-mi May 26, 2023 •

edited

Loading