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][doc] Rename LedgerInfo to avoid swagger docs conflict #19087

Closed
wants to merge 2 commits into from

Conversation

tisonkun
Copy link
Member

See apache/pulsar-site@619e9ec.

These two classes conflict in name:

  • org.apache.pulsar.common.policies.data.ManagedLedgerInternalStats.LedgerInfo
  • org.apache.bookkeeper.mledger.ManagedLedgerInfo.LedgerInfo

Since the latter one is a public interface while the former one is for internal usage, I tend to rename the former one to deduplicate.

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:

@tisonkun tisonkun self-assigned this Dec 28, 2022
@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Dec 28, 2022
Signed-off-by: tison <wander4096@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2022

Codecov Report

Merging #19087 (4ee21fa) into master (492a9c3) will decrease coverage by 0.07%.
The diff coverage is 31.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19087      +/-   ##
============================================
- Coverage     47.46%   47.38%   -0.08%     
- Complexity    10727    10728       +1     
============================================
  Files           711      711              
  Lines         69456    69461       +5     
  Branches       7452     7453       +1     
============================================
- Hits          32964    32915      -49     
- Misses        32810    32857      +47     
- Partials       3682     3689       +7     
Flag Coverage Δ
unittests 47.38% <31.25%> (-0.08%) ⬇️

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

Impacted Files Coverage Δ
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 53.73% <0.00%> (-0.09%) ⬇️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 15.06% <0.00%> (-0.04%) ⬇️
...sar/broker/service/persistent/PersistentTopic.java 60.82% <100.00%> (-1.52%) ⬇️
...lsar/client/impl/conf/ClientConfigurationData.java 96.66% <100.00%> (ø)
...java/org/apache/pulsar/proxy/stats/TopicStats.java 58.82% <0.00%> (-41.18%) ⬇️
...lsar/broker/loadbalance/impl/ThresholdShedder.java 3.27% <0.00%> (-27.87%) ⬇️
...roker/service/persistent/MessageDeduplication.java 43.23% <0.00%> (-10.49%) ⬇️
...org/apache/pulsar/broker/loadbalance/LoadData.java 58.33% <0.00%> (-8.34%) ⬇️
...g/apache/pulsar/client/impl/ConnectionHandler.java 50.00% <0.00%> (-5.32%) ⬇️
.../pulsar/broker/service/BrokerServiceException.java 47.27% <0.00%> (-4.55%) ⬇️
... and 36 more

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun requested review from hezhangjian and removed request for dlg99, AnonHxy and yuruguo December 29, 2022 00:31
@nodece
Copy link
Member

nodece commented Dec 29, 2022

Do you mean there are two LedgerInfo in the swagger.json?

@tisonkun
Copy link
Member Author

Do you mean there are two LedgerInfo in the swagger.json?

Finally there's one, but the field is different because it's randomly generated from org.apache.pulsar.common.policies.data.ManagedLedgerInternalStats.LedgerInfo or org.apache.bookkeeper.mledger.ManagedLedgerInfo.LedgerInfo.

@tisonkun
Copy link
Member Author

After a discussion with @nodece, we agree that if #19100 gets accepted, we can stop the auto sync logic or increase the interval so that this instability won't be a big problem.

Closing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants