You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/06/09 22:36:43 UTC

[GitHub] [beam] kennknowles opened a new pull request, #21783: Eliminate nullness errors in KafkaIO

kennknowles opened a new pull request, #21783:
URL: https://github.com/apache/beam/pull/21783

   **Please** add a meaningful description for your change here
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Mention the appropriate issue in your description (for example: "addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment "fixes #<ISSUE NUMBER>" instead.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


-- 
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 pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
johnjcasey commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1159055972

   This LGTM. Thank you for adding all these checks.


-- 
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] kennknowles closed pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles closed pull request #21783: Eliminate nullness errors in KafkaIO
URL: https://github.com/apache/beam/pull/21783


-- 
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 pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
johnjcasey commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1152621992

   > And in getting tests to pass, I found that they simply didn't call `setup()`, so now the tests are more accurate to real usage.
   
   That is very appreciated


-- 
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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1158457404

   Please take another look now that tests are green.
   
   Reminder of policy: when a committer is the author, they are trusted to choose the best reviewer, including not-yet-committers. I think you are the best choice for this region of the code. And thanks for the review so far!


-- 
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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1158437287

   no error in the logs, and scan failed to upload :-/


-- 
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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1158345258

   Run Python_PVR_Flink 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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1155690570

   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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1156512735

   seeing "could not find valid docker environment" across a lot of PRs, hmm


-- 
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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1156512874

   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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1156812609

   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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1158437321

   Run Python_PVR_Flink 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] kennknowles merged pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles merged PR #21783:
URL: https://github.com/apache/beam/pull/21783


-- 
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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1155744618

   Run Python_PVR_Flink 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] johnjcasey commented on a diff in pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
johnjcasey commented on code in PR #21783:
URL: https://github.com/apache/beam/pull/21783#discussion_r894556874


##########
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaExactlyOnceSink.java:
##########
@@ -691,33 +700,36 @@ void insert(int shard, ShardWriter<K, V> writer) {
    * partitions for a topic rather than for fetching messages.
    */
   private static Consumer<?, ?> openConsumer(WriteRecords<?, ?> spec) {
-    return spec.getConsumerFactoryFn()
-        .apply(
-            ImmutableMap.of(
-                ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG,
-                spec.getProducerConfig().get(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG),
-                ConsumerConfig.GROUP_ID_CONFIG,
-                spec.getSinkGroupId(),
-                ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG,
-                ByteArrayDeserializer.class,
-                ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG,
-                ByteArrayDeserializer.class));
+    SerializableFunction<Map<String, Object>, ? extends Consumer<?, ?>> consumerFactoryFn =
+        Preconditions.checkArgumentNotNull(spec.getConsumerFactoryFn());
+
+    Map<String, Object> consumerConfig = new HashMap<>();
+    Object bootStrapServersConfig =

Review Comment:
   it isn't meaningful to have Kafka config without bootstrap servers, so this should throw an exception if it is null



##########
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/ConsumerSpEL.java:
##########
@@ -117,7 +118,7 @@ public static void evaluateSeek2End(Consumer<?, ?> consumer, TopicPartition topi
   }
 
   public static void evaluateAssign(
-      Consumer<?, ?> consumer, Collection<TopicPartition> topicPartitions) {
+      @Nullable Consumer<?, ?> consumer, @Nullable Collection<TopicPartition> topicPartitions) {

Review Comment:
   I don't think these two should ever be null



-- 
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] kennknowles commented on a diff in pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on code in PR #21783:
URL: https://github.com/apache/beam/pull/21783#discussion_r894782013


##########
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/ConsumerSpEL.java:
##########
@@ -117,7 +118,7 @@ public static void evaluateSeek2End(Consumer<?, ?> consumer, TopicPartition topi
   }
 
   public static void evaluateAssign(
-      Consumer<?, ?> consumer, Collection<TopicPartition> topicPartitions) {
+      @Nullable Consumer<?, ?> consumer, @Nullable Collection<TopicPartition> topicPartitions) {

Review Comment:
   Done



##########
sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaExactlyOnceSink.java:
##########
@@ -691,33 +700,36 @@ void insert(int shard, ShardWriter<K, V> writer) {
    * partitions for a topic rather than for fetching messages.
    */
   private static Consumer<?, ?> openConsumer(WriteRecords<?, ?> spec) {
-    return spec.getConsumerFactoryFn()
-        .apply(
-            ImmutableMap.of(
-                ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG,
-                spec.getProducerConfig().get(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG),
-                ConsumerConfig.GROUP_ID_CONFIG,
-                spec.getSinkGroupId(),
-                ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG,
-                ByteArrayDeserializer.class,
-                ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG,
-                ByteArrayDeserializer.class));
+    SerializableFunction<Map<String, Object>, ? extends Consumer<?, ?>> consumerFactoryFn =
+        Preconditions.checkArgumentNotNull(spec.getConsumerFactoryFn());
+
+    Map<String, Object> consumerConfig = new HashMap<>();
+    Object bootStrapServersConfig =

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

[GitHub] [beam] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1151700512

   R: @johnjcasey 
   
   We discussed some cases.
   
   This one I did a lot more "just add an assertion" style fixes, whereas usually I like to refactor things to eliminate the possibility of any error occurring. If you can spot places where it simply doesn't make sense to have a nullable field, please 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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1152611170

   And in getting tests to pass, I found that they simply didn't call `setup()`, so now the tests are more accurate to real usage.


-- 
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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1154203474

   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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1155692079

   Run Python_PVR_Flink 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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1156672982

   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] kennknowles commented on pull request #21783: Eliminate nullness errors in KafkaIO

Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #21783:
URL: https://github.com/apache/beam/pull/21783#issuecomment-1156661362

   And across many PRs also org.apache.beam.sdk.io.pulsar.PulsarIOTest.testReadFromSimpleTopic: `Trying to claim offset 1655305408194 while last attempted was 1655305409570`


-- 
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