refactor(bigquery): resolve circular test dependency with bigquerystorage

[jinseopkim0]

Fixes #12700.

Moves all integration tests depending on java-bigquery veneer client from java-bigquerystorage to java-bigquery to eliminate the circular test dependency between the two modules.

Kokoro Configuration Updates

• Added INTEGRATION_TEST_ARGS (-Dit.test=!ITBigQueryWrite*RetryTest -Dsurefire.failIfNoSpecifiedTests=false -Dfailsafe.failIfNoSpecifiedTests=false) to bigquery-integration.cfg and bigquery-graalvm-native-presubmit.cfg.
• Note on test exclusions: This preserves existing presubmit behavior by mirroring the exact INTEGRATION_TEST_ARGS exclusions previously configured in bigquerystorage-integration.cfg and bigquerystorage-graalvm-native-presubmit.cfg to skip heavy write retry integration tests during PR presubmits.

[gemini-code-assist]

Code Review

This pull request restructures the project by moving integration tests from java-bigquerystorage to java-bigquery, updating the respective pom.xml dependencies, and adjusting the Kokoro build script to trigger java-bigquery tests when java-bigquerystorage is modified. Feedback suggests replacing the Bash string-matching array membership check with a more robust loop to avoid anti-patterns, and warns against manually editing auto-generated pom.xml files to prevent changes from being overwritten.

[jinseopkim0]

/gemini review

[gemini-code-assist]

Code Review

This pull request moves several integration tests from java-bigquerystorage to java-bigquery, updating the respective Maven configurations and the Kokoro build script to handle dependencies and module detection. Feedback suggests simplifying the duplicate-checking logic in .kokoro/common.sh using bash regex patterns and adding the arrow-memory-netty test dependency to java-bigquery's POM to prevent initialization errors during Arrow-based test execution.

[lqiu96]

Hmm, I'm not sure this change is worth the occasional occasional dependency hiccup between the two:

1. Adding features in BQStorage will mean that new ITs will need to be added to BQ. It's a bit confusing as it's not a normal structure. I think I would prefer not to combine them as they're technically two separate clients.
2. Bq and BQStorage Test times are already quite long and I worry that this would add to the long presubmit times for BQ. I think the ITs are ~16 min which isn't too bad, but do we know what the GraalVM test times are? I was looking at the CI and it seems like we may not be even running the BQ Graalvm jobs (might have been a miss on the migration)?

IIUC, BQStorage only needs BQ test scope to create the occasional table/ dataset and delete it. Maybe we can look also look to just create a shared test module between the two?

[jinseopkim0]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new integration test module, google-cloud-bigquerystorage-it, containing comprehensive integration tests for the BigQuery Storage API across v1, v1beta1, and v1beta2. The review feedback highlights several critical issues regarding resource management and test isolation in these new tests. Specifically, multiple test cases incorrectly use global client instances instead of locally configured ones, and several closeable resources—including clients, ArrowRecordBatch objects, and ExecutorService instances—are not properly managed within try-with-resources or try-finally blocks, leading to potential resource and thread leaks.

[gemini-code-assist]

There is a bug in this test where the global readClient is used to read rows instead of the newly created localClient which has the custom universe domain settings. Additionally, localClient should be managed within a try-with-resources block to prevent resource leaks.

    try (BigQueryReadClient localClient = BigQueryReadClient.create(bigQueryReadSettings)) {
      String table =
          BigQueryResource.formatTableResource(
              /* projectId= */ "bigquery-public-data",
              /* datasetId= */ "samples",
              /* tableId= */ "shakespeare");

      ReadSession session =
          localClient.createReadSession(
              /* parent= */ parentProjectId,
              /* readSession= */ ReadSession.newBuilder()
                  .setTable(table)
                  .setDataFormat(DataFormat.AVRO)
                  .build(),
              /* maxStreamCount= */ 1);

      ReadRowsRequest readRowsRequest =
          ReadRowsRequest.newBuilder().setReadStream(session.getStreams(0).getName()).build();

      long rowCount = 0;
      ServerStream<ReadRowsResponse> stream = localClient.readRowsCallable().call(readRowsRequest);
      for (ReadRowsResponse response : stream) {
        rowCount += response.getRowCount();
      }

      assertEquals(SHAKESPEARE_SAMPLE_ROW_COUNT, rowCount);
    }

References

1. Use try-with-resources to manage closeable resources in tests to ensure they are safely closed and prevent resource leaks.

[gemini-code-assist]

There is a bug in this test where the global client is used to read rows instead of the newly created localClient which has the custom universe domain settings. Additionally, localClient should be managed within a try-with-resources block to prevent resource leaks.

    try (BigQueryReadClient localClient = BigQueryReadClient.create(bigQueryReadSettings)) {
      String table =
          BigQueryResource.FormatTableResource(
              /* projectId= */ "bigquery-public-data",
              /* datasetId= */ "samples",
              /* tableId= */ "shakespeare");

      ReadSession session =
          localClient.createReadSession(
              /* parent= */ parentProjectId,
              /* readSession= */ ReadSession.newBuilder()
                  .setTable(table)
                  .setDataFormat(DataFormat.AVRO)
                  .build(),
              /* maxStreamCount= */ 1);

      ReadRowsRequest readRowsRequest =
          ReadRowsRequest.newBuilder().setReadStream(session.getStreams(0).getName()).build();

      long rowCount = 0;
      ServerStream<ReadRowsResponse> stream = localClient.readRowsCallable().call(readRowsRequest);
      for (ReadRowsResponse response : stream) {
        rowCount += response.getRowCount();
      }

      assertEquals(164_656, rowCount);
    }

References

1. Use try-with-resources to manage closeable resources in tests to ensure they are safely closed and prevent resource leaks.

[gemini-code-assist]

There is a bug in this test where the global client is used to read rows instead of the newly created localClient which has the custom universe domain settings. Additionally, localClient should be managed within a try-with-resources block to prevent resource leaks.

    try (BigQueryStorageClient localClient = BigQueryStorageClient.create(bigQueryStorageSettings)) {
      TableReference tableReference =
          TableReference.newBuilder()
              .setProjectId("bigquery-public-data")
              .setDatasetId("samples")
              .setTableId("shakespeare")
              .build();

      ReadSession session =
          localClient.createReadSession(
              /* tableReference= */ tableReference,
              /* parent= */ parentProjectId,
              /* requestedStreams= */ 1);

      StreamPosition readPosition =
          StreamPosition.newBuilder().setStream(session.getStreams(0)).build();

      ReadRowsRequest readRowsRequest =
          ReadRowsRequest.newBuilder().setReadPosition(readPosition).build();

      long rowCount = 0;
      ServerStream<ReadRowsResponse> stream = localClient.readRowsCallable().call(readRowsRequest);
      for (ReadRowsResponse response : stream) {
        rowCount += response.getRowCount();
      }

      assertEquals(164_656, rowCount);
    }

References

1. Use try-with-resources to manage closeable resources in tests to ensure they are safely closed and prevent resource leaks.

[gemini-code-assist]

The ArrowRecordBatch object should be managed within a try-with-resources block to ensure it is closed properly and direct memory is not leaked, even if an exception is thrown during row processing. Additionally, root.clear() should be executed in a finally block to guarantee the root vectors are cleared.

Suggested change

	org.apache.arrow.vector.ipc.message.ArrowRecordBatch deserializedBatch =
	MessageSerializer.deserializeRecordBatch(
	new ReadChannel(
	new ByteArrayReadableSeekableByteChannel(
	batch.getSerializedRecordBatch().toByteArray())),
	allocator);
	loader.load(deserializedBatch);
	// Release buffers from batch (they are still held in the vectors in root).
	deserializedBatch.close();
	batchConsumer.accept(root);
	// Release buffers from vectors in root.
	root.clear();
	try (org.apache.arrow.vector.ipc.message.ArrowRecordBatch deserializedBatch =
	MessageSerializer.deserializeRecordBatch(
	new ReadChannel(
	new ByteArrayReadableSeekableByteChannel(
	batch.getSerializedRecordBatch().toByteArray())),
	allocator)) {
	loader.load(deserializedBatch);
	batchConsumer.accept(root);
	} finally {
	root.clear();
	}

References

1. Use try-with-resources to manage closeable resources in tests to ensure they are safely closed and prevent resource leaks.

[gemini-code-assist]

The BigQueryStorageClient should be managed within a try-with-resources block to ensure it is closed properly even if assertions fail or exceptions are thrown during the test.

    try (BigQueryStorageClient localClient = BigQueryStorageClient.create(bigQueryStorageSettings)) {
      TableReference tableReference =
          TableReference.newBuilder()
              .setProjectId("bigquery-public-data")
              .setDatasetId("samples")
              .setTableId("shakespeare")
              .build();

      UnauthenticatedException e =
          assertThrows(
              UnauthenticatedException.class,
              () ->
                  localClient.createReadSession(
                      /* tableReference= */ tableReference,
                      /* parent= */ parentProjectId,
                      /* requestedStreams= */ 1));
      assertThat(
              (e.getMessage()
                  .contains("does not match the universe domain found in the credentials")))
          .isTrue();
    }

References

1. Use try-with-resources to manage closeable resources in tests to ensure they are safely closed and prevent resource leaks.

[gemini-code-assist]

The BigQueryReadClient should be managed within a try-with-resources block to ensure it is closed properly even if assertions fail or exceptions are thrown during the test.

    try (BigQueryReadClient localClient = BigQueryReadClient.create(bigQueryReadSettings)) {
      String table =
          BigQueryResource.FormatTableResource(
              /* projectId= */ "bigquery-public-data",
              /* datasetId= */ "samples",
              /* tableId= */ "shakespeare");

      UnauthenticatedException e =
          assertThrows(
              UnauthenticatedException.class,
              () ->
                  localClient.createReadSession(
                      /* parent= */ parentProjectId,
                      /* readSession= */ ReadSession.newBuilder()
                          .setTable(table)
                          .setDataFormat(DataFormat.AVRO)
                          .build(),
                      /* maxStreamCount= */ 1));
      assertThat(
              (e.getMessage()
                  .contains("does not match the universe domain found in the credentials")))
          .isTrue();
    }

References

1. Use try-with-resources to manage closeable resources in tests to ensure they are safely closed and prevent resource leaks.

[gemini-code-assist]

Overwriting the shared static readClient field with a custom-configured client in a test method leaks the previously initialized global client and can cause side effects for other tests. Use a local client variable managed within a try-with-resources block instead.

    try (BigQueryReadClient localClient = BigQueryReadClient.create(bigQueryReadSettings)) {
      assertTrue(
          localClient.getStub().getStubSettings().getBackgroundExecutorProvider()
              instanceof InstantiatingExecutorProvider);
      assertEquals(
          14,
          ((InstantiatingExecutorProvider)
                  localClient.getStub().getStubSettings().getBackgroundExecutorProvider())
              .getExecutorThreadCount());
      String table =
          BigQueryResource.formatTableResource(
              /* projectId= */ "bigquery-public-data",
              /* datasetId= */ "samples",
              /* tableId= */ "shakespeare");

      ReadSession session =
          localClient.createReadSession(
              /* parent= */ parentProjectId,
              /* readSession= */ ReadSession.newBuilder()
                  .setTable(table)
                  .setDataFormat(DataFormat.AVRO)
                  .build(),
              /* maxStreamCount= */ 1);
      assertEquals(
          1,
          session.getStreamsCount(),
          String.format(
              "Did not receive expected number of streams for table '%s' CreateReadSession"
                  + " response:%n%s",
              table, session.toString()));

      ReadRowsRequest readRowsRequest =
          ReadRowsRequest.newBuilder().setReadStream(session.getStreams(0).getName()).build();

      long rowCount = 0;
      ServerStream<ReadRowsResponse> stream = localClient.readRowsCallable().call(readRowsRequest);
      for (ReadRowsResponse response : stream) {
        rowCount += response.getRowCount();
      }

      assertEquals(SHAKESPEARE_SAMPLE_ROW_COUNT, rowCount);
    }

References

1. Use try-with-resources to manage closeable resources in tests to ensure they are safely closed and prevent resource leaks.

[gemini-code-assist]

The ExecutorService is never shut down, which causes thread leaks in the test JVM. Ensure executor.shutdown() is always called by wrapping the execution in a try-finally block.

Suggested change

	ExecutorService executor = Executors.newFixedThreadPool(tasks.size());
	List<Future<Long>> results = executor.invokeAll(tasks);
	long rowCount = 0;
	for (Future<Long> result : results) {
	rowCount += result.get();
	}
	assertEquals(313_797_035, rowCount);
	ExecutorService executor = Executors.newFixedThreadPool(tasks.size());
	try {
	List<Future<Long>> results = executor.invokeAll(tasks);
	long rowCount = 0;
	for (Future<Long> result : results) {
	rowCount += result.get();
	}
	assertEquals(313_797_035, rowCount);
	} finally {
	executor.shutdown();
	}

[gemini-code-assist]

If executor.invokeAll(tasks) throws an exception, executor.shutdown() is skipped, causing thread leaks. Wrap the execution in a try-finally block to guarantee shutdown.

    ExecutorService executor = Executors.newFixedThreadPool(tasks.size());
    try {
      List<Future<Long>> results = executor.invokeAll(tasks);
      long rowCount = 0;
      for (Future<Long> result : results) {
        rowCount += result.get();
      }
      assertEquals(313_797_035, rowCount);
    } finally {
      executor.shutdown();
    }

[gemini-code-assist]

If executor.invokeAll(tasks) throws an exception, executor.shutdown() is skipped, causing thread leaks. Wrap the execution in a try-finally block to guarantee shutdown.

    ExecutorService executor = Executors.newFixedThreadPool(tasks.size());
    try {
      List<Future<Long>> results = executor.invokeAll(tasks);
      long rowCount = 0;
      for (Future<Long> result : results) {
        rowCount += result.get();
      }
      assertEquals(313_797_035, rowCount);
    } finally {
      executor.shutdown();
    }

[blakeli0]

BQStorage only needs BQ test scope to create the occasional table/ dataset and delete it

I agree with Lawrence mostly. Can we investigate if this is a must-have? Bigquerystorage is a dependency of Bigquery, so theoretically it should not use a downstream library to test its own functionalities. For example, auth does not have any tests that requires gax, gax does not have tests that requires GAPIC libraries. Instead of migrating tests to a downstream library, I would prefer to delete unnecessary tests or migrate the tests to not use downstream libraries.

[sonarqubecloud]

Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud