Skip to content

[fix][sql] Remove useless configuration for Pulsar SQL#20605

Merged
Technoboy- merged 3 commits intoapache:masterfrom
gaoran10:sql-remove-uesless-config-num-thread-ml
Jun 29, 2023
Merged

[fix][sql] Remove useless configuration for Pulsar SQL#20605
Technoboy- merged 3 commits intoapache:masterfrom
gaoran10:sql-remove-uesless-config-num-thread-ml

Conversation

@gaoran10
Copy link
Contributor

@gaoran10 gaoran10 commented Jun 19, 2023

Motivation

In PR #14506, we remove the useless config managedLedgerNumWorkerThreads
but we didn't remove the config from the config file, if users use this configuration, the Pulsar SQL can't startup due to strictConfig mode, remove this useless configuration for Pulsar SQL.

image
// at this point all config file properties should be used
// so we can calculate the unused properties
unusedProperties.keySet().removeAll(configurationFactory.getUsedProperties());

for (String key : unusedProperties.keySet()) {
    Message message = new Message(format("Configuration property '%s' was not used", key));
    (strictConfig ? errors : warnings).add(message);
}

// If there are configuration errors, fail-fast to keep output clean
if (!errors.isEmpty()) {
    throw new ApplicationConfigurationException(errors, warnings);
}

Modifications

Remove useless configuration pulsar.managed-ledger-num-worker-threads from the config file.

Verifying this change

Add tests for the annotated configurations.

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: gaoran10#28

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 19, 2023
@gaoran10 gaoran10 self-assigned this Jun 19, 2023
@gaoran10 gaoran10 requested a review from hangc0276 June 19, 2023 09:24
@Technoboy- Technoboy- added this to the 3.1.0 milestone Jun 20, 2023
@Technoboy- Technoboy- closed this Jun 20, 2023
@Technoboy- Technoboy- reopened this Jun 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.71%. Comparing base (8125d1f) to head (d5b172f).
Report is 1223 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20605      +/-   ##
============================================
+ Coverage     72.29%   72.71%   +0.42%     
- Complexity    31951    32305     +354     
============================================
  Files          1854     1867      +13     
  Lines        138462   140417    +1955     
  Branches      15221    15427     +206     
============================================
+ Hits         100095   102106    +2011     
+ Misses        30384    30249     -135     
- Partials       7983     8062      +79     
Flag Coverage Δ
inttests 24.40% <ø> (?)
systests 25.32% <ø> (?)
unittests 72.46% <ø> (+0.17%) ⬆️

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

see 208 files with indirect coverage changes

@Technoboy- Technoboy- closed this Jun 29, 2023
@Technoboy- Technoboy- reopened this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants