Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][io] KCA: 'desanitize' topic name for the pulsar's ctx calls #19756

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

dlg99
Copy link
Contributor

@dlg99 dlg99 commented Mar 9, 2023

Motivation

Some kafka connectors use topic name to e.g. create table names but pulsar's topic URI does not comply with requirements of the table names. This problems either handled by the connector or by KCA if sanitizeTopicName is set to true.
Some kafka connectors use pause/resume API, in that case sanitized name has to be converted back to original URI to pass to Pulsar's components.

Modifications

Provided a way to resolve original topic URI.

Added unit test.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added unit test.

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

NO

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: dlg99#10

@dlg99 dlg99 self-assigned this Mar 9, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 9, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #19756 (dc57e44) into master (af1360f) will increase coverage by 35.61%.
The diff coverage is 82.35%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19756       +/-   ##
=============================================
+ Coverage     26.40%   62.02%   +35.61%     
- Complexity     6427    25758    +19331     
=============================================
  Files          1597     1848      +251     
  Lines        123682   135882    +12200     
  Branches      13511    14954     +1443     
=============================================
+ Hits          32658    84279    +51621     
+ Misses        86336    43810    -42526     
- Partials       4688     7793     +3105     
Flag Coverage Δ
inttests 24.53% <ø> (-0.04%) ⬇️
systests 25.12% <ø> (?)
unittests 59.37% <82.35%> (+41.97%) ⬆️

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

Impacted Files Coverage Δ
...ache/pulsar/io/kafka/connect/KafkaConnectSink.java 73.06% <70.00%> (ø)
...r/io/kafka/connect/PulsarKafkaSinkTaskContext.java 64.03% <100.00%> (ø)
...ce/ConsistentHashingStickyKeyConsumerSelector.java 76.92% <0.00%> (-3.85%) ⬇️
...in/java/org/apache/pulsar/common/api/AuthData.java 71.42% <0.00%> (ø)
.../apache/pulsar/broker/namespace/LookupOptions.java 87.50% <0.00%> (ø)
.../apache/pulsar/common/naming/SystemTopicNames.java 55.55% <0.00%> (ø)
...apache/pulsar/common/util/SafeCollectionUtils.java 0.00% <0.00%> (ø)
...pache/pulsar/common/configuration/BindAddress.java 22.22% <0.00%> (ø)
...lsar/client/impl/conf/ReaderConfigurationData.java 81.39% <0.00%> (ø)
...r/client/admin/internal/data/AuthPoliciesImpl.java 65.21% <0.00%> (ø)
... and 1348 more

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.

LGTM

@eolivelli eolivelli merged commit d4930a3 into apache:master Mar 9, 2023
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Mar 9, 2023
dlg99 added a commit to dlg99/pulsar that referenced this pull request Mar 10, 2023
dlg99 added a commit to dlg99/pulsar that referenced this pull request Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

3 participants