Skip to content

Conversation

@aymkhalil
Copy link
Contributor

Fixes #18507

Motivation

When a user updates inputSpect of a sink by providing both --inputs and --input-specs, exiting consumerConfig will be wiped out.

Modifications

On the validateUpdate code path, there is a logic that populates the inputSpecs inside the newConfig to empty consumerConfig. This will overwrite the existing specs when later on the methods attempts a merge and will also merge in the hard coded consumer configs ignoring any new values the user might've provided. By only setting the consumerConfig to a default value if such config didn't exit, the behavior is similar to what would happen if the user did the update, without providing the --inputs specs (which is, newConfig consumer config will overwrite exiting ones.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: aymkhalil#4

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 27, 2022
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2022

Codecov Report

Merging #19082 (2d82081) into master (492a9c3) will increase coverage by 0.33%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19082      +/-   ##
============================================
+ Coverage     47.46%   47.79%   +0.33%     
+ Complexity    10727     9574    -1153     
============================================
  Files           711      631      -80     
  Lines         69456    59691    -9765     
  Branches       7452     6215    -1237     
============================================
- Hits          32964    28531    -4433     
+ Misses        32810    28066    -4744     
+ Partials       3682     3094     -588     
Flag Coverage Δ
unittests 47.79% <0.00%> (+0.33%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 15.06% <0.00%> (-0.04%) ⬇️
.../pulsar/broker/service/SharedConsumerAssignor.java 3.70% <0.00%> (-64.82%) ⬇️
...java/org/apache/pulsar/proxy/stats/TopicStats.java 58.82% <0.00%> (-41.18%) ⬇️
...apache/pulsar/broker/service/EntryAndMetadata.java 0.00% <0.00%> (-40.75%) ⬇️
...roker/service/persistent/MessageDeduplication.java 43.23% <0.00%> (-10.49%) ⬇️
...sistent/PersistentDispatcherMultipleConsumers.java 51.04% <0.00%> (-8.16%) ⬇️
...g/apache/pulsar/client/impl/ConnectionHandler.java 50.00% <0.00%> (-5.32%) ⬇️
.../pulsar/broker/service/BrokerServiceException.java 49.09% <0.00%> (-2.73%) ⬇️
.../org/apache/pulsar/broker/admin/v2/Namespaces.java 58.54% <0.00%> (-1.50%) ⬇️
...a/org/apache/pulsar/client/impl/RawReaderImpl.java 81.73% <0.00%> (-0.97%) ⬇️
... and 112 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/connector doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Pulsar Admin sink update can potentially wipe out "inputSpecs"

6 participants