Skip to content

KAFKA-14848: KafkaConsumer incorrectly passes locally-scoped deserializers to FetchConfig#13452

Merged
guozhangwang merged 1 commit into
apache:trunkfrom
kirktrue:KAFKA-14848-fix-fetchconfig-npe
Mar 27, 2023
Merged

KAFKA-14848: KafkaConsumer incorrectly passes locally-scoped deserializers to FetchConfig#13452
guozhangwang merged 1 commit into
apache:trunkfrom
kirktrue:KAFKA-14848-fix-fetchconfig-npe

Conversation

@kirktrue

Copy link
Copy Markdown
Contributor

Fix for a NPE bug that was caused by referring to a local variable and not the instance variable of the deserializers.

See fix suggestion from @rayokota: #13425 (comment)

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

…ers to FetchConfig

Fix for a NPE bug that was caused by referring to a local variable and not the instance variable of the deserializers.

Co-Authored-By: Robert Yokota <1761488+rayokota@users.noreply.github.com>
@kirktrue kirktrue changed the title KAFKA-14848: KafkaConsumer incorrectly passes locally-scoped serializers to FetchConfig KAFKA-14848: KafkaConsumer incorrectly passes locally-scoped deserializers to FetchConfig Mar 26, 2023

@rayokota rayokota left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kirktrue , LGTM!

@chia7712 chia7712 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

this.clientRackId = config.getString(ConsumerConfig.CLIENT_RACK_CONFIG);
this.keyDeserializer = keyDeserializer;
this.valueDeserializer = valueDeserializer;
this.keyDeserializer = Objects.requireNonNull(keyDeserializer, "Message key deserializer provided to FetchConfig should not be null");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: how about calling another constructor to reduce duplicate code?

        this(config.getInt(ConsumerConfig.FETCH_MIN_BYTES_CONFIG),
                config.getInt(ConsumerConfig.FETCH_MAX_BYTES_CONFIG),
                config.getInt(ConsumerConfig.FETCH_MAX_WAIT_MS_CONFIG)
                        ...)

@kirktrue
kirktrue marked this pull request as ready for review March 27, 2023 16:06
@guozhangwang

Copy link
Copy Markdown
Contributor

LGTM. Merging to trunk to unblock @rayokota and SR, please feel free to address @chia7712 's comments in a follow-up PR.

@guozhangwang
guozhangwang merged commit 31440b0 into apache:trunk Mar 27, 2023
@kirktrue
kirktrue deleted the KAFKA-14848-fix-fetchconfig-npe branch March 27, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants