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.

Sign up for GitHub

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

Jump to bottom

[feat][broker] PIP-180 Part VI: Add ShadowManagedLedgerImpl #18265

Merged

Jason918 merged 11 commits into apache:master from Jason918:pip-180-dev

Nov 29, 2022

Contributor

Jason918 commented Oct 31, 2022 •

edited

Loading

Master Issue: #16153

Motivation

After this PR, the basic function of PIP-180 shadow topic will be ready.

Modifications

Add ShadowManagedLedgerImpl

Verifying this change

Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

see ShadowTopicTest.java

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

If the box was checked, please highlight the changes

Documentation

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

Matching PR in forked repository

PR in forked repository: Jason918#12

github-actions bot added the doc-required label

Jason918 self-assigned this

Jason918 added type/feature area/broker labels

Jason918 added this to the 2.12.0 milestone

Jason918 requested review from eolivelli, codelipenghui, zymap, 315157973, AnonHxy, mattisonchao, michaeljmarshall and hangc0276

October 31, 2022 08:09

Jason918 mentioned this pull request

[feat][broker] PIP-180 Part VI: Add ShadowManagedLedgerImpl Jason918/pulsar#12

Closed

Jason918 added the ready-to-test label

AnonHxy reviewed

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java Show resolved Hide resolved

Contributor Author

Jason918 commented Nov 15, 2022

/pulsarbot run-failure-checks

Contributor Author

Jason918 commented Nov 15, 2022

@codelipenghui @eolivelli @michaeljmarshall @lhotari @mattisonchao PTAL

Jason918 requested review from AnonHxy, HQebupt and lhotari

November 15, 2022 02:36

codecov-commenter commented Nov 15, 2022 •

edited

Loading

Codecov Report

Merging #18265 (536f10a) into master (b579bb8) will decrease coverage by 0.45%.
The diff coverage is 16.34%.

@@             Coverage Diff              @@
##             master   #18265      +/-   ##
============================================
- Coverage     47.51%   47.06%   -0.46%     
+ Complexity    10520    10457      -63     
============================================
  Files           698      698              
  Lines         68079    68323     +244     
  Branches       7280     7332      +52     
============================================
- Hits          32351    32157     -194     
- Misses        32151    32565     +414     
- Partials       3577     3601      +24

Flag	Coverage Δ
unittests	`47.06% <16.34%> (-0.46%)`	⬇️

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

Impacted Files	Coverage Δ
...apache/bookkeeper/mledger/ManagedLedgerConfig.java	`71.60% <0.00%> (-0.90%)`	⬇️
.../org/apache/bookkeeper/mledger/impl/MetaStore.java	`100.00% <ø> (ø)`
...okkeeper/mledger/impl/ShadowManagedLedgerImpl.java	`0.00% <0.00%> (ø)`
...loadbalance/impl/LeastResourceUsageWithWeight.java	`0.00% <0.00%> (ø)`
...lsar/broker/loadbalance/impl/ThresholdShedder.java	`3.27% <0.00%> (-44.44%)`	⬇️
...ersistentStreamingDispatcherMultipleConsumers.java	`0.00% <0.00%> (ø)`
...istentStreamingDispatcherSingleActiveConsumer.java	`0.00% <0.00%> (ø)`
...ervice/streamingdispatch/StreamingEntryReader.java	`0.00% <0.00%> (ø)`
...va/org/apache/pulsar/client/impl/ProducerImpl.java	`17.00% <0.00%> (-0.02%)`	⬇️
...rg/apache/pulsar/broker/service/BrokerService.java	`57.40% <20.00%> (-0.55%)`	⬇️
... and 57 more

HQebupt approved these changes

View reviewed changes

Contributor

HQebupt left a comment

LGTM

AnonHxy reviewed

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java Show resolved Hide resolved

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java Outdated Show resolved Hide resolved

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java Show resolved Hide resolved

AnonHxy approved these changes

View reviewed changes

Contributor

AnonHxy commented Nov 16, 2022

LGTM with minor comments

Jason918 force-pushed the pip-180-dev branch from 9295d45 to eb300b8 Compare

November 22, 2022 08:32

codelipenghui reviewed

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java Outdated

                   public ShadowManagedLedgerImpl(ManagedLedgerFactoryImpl factory, BookKeeper bookKeeper,
                                                  MetaStore store, ManagedLedgerConfig config,
                                                  OrderedScheduler scheduledExecutor,
                                                  String name, final Supplier<Boolean> mlOwnershipChecker) {
                       super(factory, bookKeeper, store, config, scheduledExecutor, name, mlOwnershipChecker);
-                      this.shadowSource = TopicName.get(config.getShadowSource());
-                      this.sourceMLName = shadowSource.getPersistenceNamingEncoding();
+                      if (config.getTopicName().isPartitioned() && TopicName.getPartitionIndex(config.getShadowSource()) == -1) {

Contributor

codelipenghui Nov 23, 2022

Why the name is not able to be used? It's a little confusing that we need to introduce the topic name in ManagedLedgerConfig

Or maybe we can introduce a properties?

Contributor Author

Jason918 Nov 25, 2022

Why the name is not able to be used? It's a little confusing that we need to introduce the topic name in ManagedLedgerConfig

Good point.

We need to know the TopicName of the source topic in the shadow topic. But the name here is in the format of PersistenceNamingEncoding.
I get that we'd better not introduce the concept of "topic" into managed-ledger. I will move this logic to the "broker" layer. So that the source topic name is in properties and the managedLedgerName of the source topic is stored in the new field ManagedLedgerConfig.shadowSourceName.

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java

+                                  sourceLedgersStat, stat);
+                      }
+                      sourceLedgersStat = stat;

Contributor

codelipenghui Nov 24, 2022

Do we need to check the data version?
Maybe the watcher gets a notification first, and then we get the response from get managed ledger info operation. Will we add the deleted ledger back?

Contributor Author

Jason918 Nov 25, 2022

Nice catch. Added a version check.

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java Outdated

+                      sourceLedgersStat = stat;
+                      if (mlInfo.hasTerminatedPosition()) {
+                          state = State.Terminated;

Contributor

codelipenghui Nov 24, 2022

Do you need Terminated state for a shadow ledger? If the source topic changes to the terminated state, the shadow topic will not get any new messages.

Contributor Author

Jason918 Nov 25, 2022

Not needed actually, just keeping the original logic at first.
After another thought, we should remove this, because the source topic can be terminated but the replication to the shadow topic should go on.
So comes these updates:

Removed Terminated state from ShadowManagedLedger.
Overrides asyncTerminate and return fail directly.

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java Outdated

Comment on lines 240 to 244

+                      if (state == State.Terminated) {
+                          addOperation.failed(new ManagedLedgerException.ManagedLedgerTerminatedException(
+                                  "Managed ledger was already terminated"));
+                          return;
+                      }

Contributor

codelipenghui Nov 24, 2022

Looks like we don't need to check if the topic is terminated.
And if the broker receives the zookeeper notification first, then the shadow topic changes to the terminated state. But the shadow replicator hasn't reached the end of the source topic. In this case, the consumer connected to the shadow topic will have a different view.

Contributor Author

Jason918 Nov 25, 2022

Yes, exactly.

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java Outdated

+                          currentLedgerEntries = position.getEntryId();
+                          currentLedgerSize += addOperation.data.readableBytes();
+                          addOperation.initiateShadowWrite();
+                      }

Contributor

codelipenghui Nov 24, 2022

How about the position's ledger ID that is not equal to the current ledger ID? It looks like the addOperation will never be complete.

Contributor Author

Jason918 Nov 25, 2022

Fixed, and add ut in ShadowManagedLedgerImplTest

Contributor Author

Jason918 commented Nov 25, 2022

@codelipenghui Comments all addressed. PTAL.


          PIP-180 Part VI, Add ShadowManagedLedgerImpl

5d5189a

- finish processSourceManagedLedgerInfo
- finish internalAsyncAddEntry
- ShadowTopicTest pass
- ShadowReplicatorTest pass

Add test cases

Jason918 added 10 commits

November 28, 2022 14:55


          fix test

07d9649


          fix testCompactionRetentionOnTopicCreationWithTopicPolicies

1f570e1


          fix testBlockIfQueueFullWhenChunking

cb1fc40


          address comments

f31e1c0


          move topic name related logic in managed-ledger to broker layer.


          Add version check when process mlInfo from sourceML

1dd5c7b


          Removed Terminated state from ShadowManagedLedger.

48ebdeb


          Add ShadowManagedLedgerImplTest

b9a2916


          fix header

d11e502


          fix compile

536f10a

Jason918 force-pushed the pip-180-dev branch from 69b8222 to 536f10a Compare

November 28, 2022 06:56

codelipenghui approved these changes

View reviewed changes

Jason918 merged commit 5b062f3 into apache:master

lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request


          [feat][broker] PIP-180 Part VI: Add ShadowManagedLedgerImpl (apache#1…

8cae91b

…8265)

lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request


          [feat][broker] PIP-180 Part VI: Add ShadowManagedLedgerImpl (apache#1…

4f7cacc

…8265)

Contributor

momo-jun commented Jan 18, 2023

Hi @Jason918, do you have any updates for documenting this feature?
Since 2.11 has been released, now it's good timing to start the doc update in the next version.

This was referenced Feb 7, 2023

[branch-2.10] [fix] [ml] Change the modifier of some variables in ManagedLedgerImpl to default to resolve compilation errors #19452

Closed

[branch-2.11] [fix] [ml] Change the modifier of some variables in ManagedLedgerImpl to default to resolve compilation errors #19453

Closed

michaeljmarshall mentioned this pull request

[cleanup][broker] Cleanup ManagedLedgerImpl's nouse method: isLedgersReadonly #19513

Merged

15 tasks

Contributor Author

Jason918 commented Feb 17, 2023

Hi @Jason918, do you have any updates for documenting this feature?
Since 2.11 has been released, now it's good timing to start the doc update in the next version.

@momo-jun Thanks for the reminding, I will try to come up with the doc update before the 3.0.0 release.

Jason918 deleted the pip-180-dev branch

February 17, 2023 02:26

Member

StevenLuMT commented Feb 17, 2023

Hi @Jason918, do you have any updates for documenting this feature?
Since 2.11 has been released, now it's good timing to start the doc update in the next version.

@momo-jun Thanks for the reminding, I will try to come up with the doc update before the 3.0.0 release.

@momo-jun I have communicated with @Jason918 , I'll help him update the documentation

Contributor

momo-jun commented Feb 17, 2023

Thanks @Jason918 @StevenLuMT. Feel free to ping me if you need any assistance with docs.

Member

StevenLuMT commented Feb 17, 2023

Thanks @Jason918 @StevenLuMT. Feel free to ping me if you need any assistance with docs.

Thanks @momo-jun ,my WeChat ID is 863199780, add me, keep fast communication

RobertIndie mentioned this pull request

PIP-180: Shadow Topic, an alternative way to support readonly topic ownership. #16153

Open

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

Reviewers

codelipenghui codelipenghui approved these changes

AnonHxy AnonHxy approved these changes

HQebupt HQebupt approved these changes

eolivelli Awaiting requested review from eolivelli

zymap Awaiting requested review from zymap

315157973 Awaiting requested review from 315157973

mattisonchao Awaiting requested review from mattisonchao

michaeljmarshall Awaiting requested review from michaeljmarshall

hangc0276 Awaiting requested review from hangc0276

lhotari Awaiting requested review from lhotari

Labels

area/broker doc-required ready-to-test type/feature