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 2023/08/16 17:38:51 UTC
[GitHub] [beam] ahmedabu98 opened a new pull request, #28032: Add more batch Storage Write API tests
ahmedabu98 opened a new pull request, #28032:
URL: https://github.com/apache/beam/pull/28032
(no comment)
--
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] ahmedabu98 commented on pull request #28032: Add more batch Storage Write API tests
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28032:
URL: https://github.com/apache/beam/pull/28032#issuecomment-1681035286
Run Kotlin_Examples 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] ahmedabu98 commented on a diff in pull request #28032: Add more batch Storage Write API tests
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #28032:
URL: https://github.com/apache/beam/pull/28032#discussion_r1296311554
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOWriteTest.java:
##########
@@ -302,15 +303,15 @@ public void testWriteDynamicDestinations() throws Exception {
}
@Test
- public void testWriteDynamicDestinationsBatchWithSchemas() throws Exception {
- writeDynamicDestinations(true, false);
+ public void testWriteDynamicDestinationsWithAutoSharding() throws Exception {
+ assumeTrue(useStreaming);
+ assumeTrue(!useStorageApiApproximate); // STORAGE_API_AT_LEAST_ONCE ignores auto-sharding
+ writeDynamicDestinations(false, false);
}
@Test
- public void testWriteDynamicDestinationsStreamingWithAutoSharding() throws Exception {
- assumeTrue(useStreaming);
- assumeTrue(!useStorageApiApproximate); // STORAGE_API_AT_LEAST_ONCE ignores auto-sharding
- writeDynamicDestinations(true, true);
+ public void testWriteDynamicDestinationsWithBeamSchemas() throws Exception {
Review Comment:
To be more accurate:
1. This tests both batch and streaming modes
2. `withSchemas` actually means using with Beam Schema
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOWriteTest.java:
##########
@@ -302,15 +303,15 @@ public void testWriteDynamicDestinations() throws Exception {
}
@Test
- public void testWriteDynamicDestinationsBatchWithSchemas() throws Exception {
- writeDynamicDestinations(true, false);
+ public void testWriteDynamicDestinationsWithAutoSharding() throws Exception {
+ assumeTrue(useStreaming);
+ assumeTrue(!useStorageApiApproximate); // STORAGE_API_AT_LEAST_ONCE ignores auto-sharding
+ writeDynamicDestinations(false, false);
Review Comment:
Good catch, this was unintended. Will 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] johnjcasey commented on a diff in pull request #28032: Add more batch Storage Write API tests
Posted by "johnjcasey (via GitHub)" <gi...@apache.org>.
johnjcasey commented on code in PR #28032:
URL: https://github.com/apache/beam/pull/28032#discussion_r1296280323
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOWriteTest.java:
##########
@@ -302,15 +303,15 @@ public void testWriteDynamicDestinations() throws Exception {
}
@Test
- public void testWriteDynamicDestinationsBatchWithSchemas() throws Exception {
- writeDynamicDestinations(true, false);
+ public void testWriteDynamicDestinationsWithAutoSharding() throws Exception {
+ assumeTrue(useStreaming);
+ assumeTrue(!useStorageApiApproximate); // STORAGE_API_AT_LEAST_ONCE ignores auto-sharding
+ writeDynamicDestinations(false, false);
}
@Test
- public void testWriteDynamicDestinationsStreamingWithAutoSharding() throws Exception {
- assumeTrue(useStreaming);
- assumeTrue(!useStorageApiApproximate); // STORAGE_API_AT_LEAST_ONCE ignores auto-sharding
- writeDynamicDestinations(true, true);
+ public void testWriteDynamicDestinationsWithBeamSchemas() throws Exception {
Review Comment:
Why did we change the name of this?
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOWriteTest.java:
##########
@@ -1329,8 +1330,12 @@ static class SchemaPojo {
}
@Test
- public void testSchemaWriteLoads() throws Exception {
- assumeTrue(!useStreaming);
+ public void testBatchSchemaWriteLoads() throws Exception {
+ // This test is actually for batch!
+ // We just invert useStreaming because we need it as true to test
+ // STORAGE_API_AT_LEAST_ONCE,
+ assumeTrue(useStreaming);
Review Comment:
It looks like you don't invert useStreaming, is that intentional?
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOWriteTest.java:
##########
@@ -302,15 +303,15 @@ public void testWriteDynamicDestinations() throws Exception {
}
@Test
- public void testWriteDynamicDestinationsBatchWithSchemas() throws Exception {
- writeDynamicDestinations(true, false);
+ public void testWriteDynamicDestinationsWithAutoSharding() throws Exception {
+ assumeTrue(useStreaming);
+ assumeTrue(!useStorageApiApproximate); // STORAGE_API_AT_LEAST_ONCE ignores auto-sharding
+ writeDynamicDestinations(false, false);
Review Comment:
Why did we swap this from true, true to false, false
--
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] johnjcasey merged pull request #28032: Add more batch Storage Write API tests
Posted by "johnjcasey (via GitHub)" <gi...@apache.org>.
johnjcasey merged PR #28032:
URL: https://github.com/apache/beam/pull/28032
--
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] ahmedabu98 commented on pull request #28032: Add more batch Storage Write API tests
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #28032:
URL: https://github.com/apache/beam/pull/28032#issuecomment-1681042542
R: @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] ahmedabu98 commented on a diff in pull request #28032: Add more batch Storage Write API tests
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #28032:
URL: https://github.com/apache/beam/pull/28032#discussion_r1296309909
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOWriteTest.java:
##########
@@ -1329,8 +1330,12 @@ static class SchemaPojo {
}
@Test
- public void testSchemaWriteLoads() throws Exception {
- assumeTrue(!useStreaming);
+ public void testBatchSchemaWriteLoads() throws Exception {
+ // This test is actually for batch!
+ // We just invert useStreaming because we need it as true to test
+ // STORAGE_API_AT_LEAST_ONCE,
+ assumeTrue(useStreaming);
Review Comment:
I meant that we're meant to use `!useStreaming` for batch tests, but used `useStreaming` instead. Will update comments to clarify
--
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 #28032: Add more batch Storage Write API tests
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28032:
URL: https://github.com/apache/beam/pull/28032#issuecomment-1681046244
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