[fix][broker]Make LedgerOffloaderFactory can load the old nar. #19913

horizonzy · 2023-03-23T16:35:05Z

Motivation

After #13833, we change the signature of LedgerOffloaderFactory#create.

Before: no offloaderStats.

    default T create(OffloadPoliciesImpl offloadPolicies,
                     Map<String, String> userMetadata,
                     SchemaStorage schemaStorage,
                     OrderedScheduler scheduler)
            throws IOException {
        return create(offloadPolicies, userMetadata, scheduler);
    }

After: add offloaderStats.

    default T create(OffloadPoliciesImpl offloadPolicies,
                     Map<String, String> userMetadata,
                     SchemaStorage schemaStorage,
                     OrderedScheduler scheduler,
                     LedgerOffloaderStats offloaderStats)
            throws IOException {
        return create(offloadPolicies, userMetadata, scheduler, offloaderStats);
    }

And in PulsarService#createManagedLedgerOffloader line_1402, it changes the way to create an offloader.

Before: no offloaderStats

                    return offloaderFactory.create(
                        offloadPolicies,
                        ImmutableMap.of(
                            LedgerOffloader.METADATA_SOFTWARE_VERSION_KEY.toLowerCase(), PulsarVersion.getVersion(),
                            LedgerOffloader.METADATA_SOFTWARE_GITSHA_KEY.toLowerCase(), PulsarVersion.getGitSha(),
                            LedgerOffloader.METADATA_PULSAR_CLUSTER_NAME.toLowerCase(), config.getClusterName()
                        ),
                        schemaStorage,
                        getOffloaderScheduler(offloadPolicies));

After: add offloaderStats

return offloaderFactory.create(
                        offloadPolicies,
                        ImmutableMap.of(
                            LedgerOffloader.METADATA_SOFTWARE_VERSION_KEY.toLowerCase(), PulsarVersion.getVersion(),
                            LedgerOffloader.METADATA_SOFTWARE_GITSHA_KEY.toLowerCase(), PulsarVersion.getGitSha(),
                            LedgerOffloader.METADATA_PULSAR_CLUSTER_NAME.toLowerCase(), config.getClusterName()
                        ),
                        schemaStorage, getOffloaderScheduler(offloadPolicies), this.offloaderStats);

But some users may use the old version LedgerOffloaderFactory in the offloader nar, when pulsar load the offloader nar, it will use the LedgerOffloaderFactory in the offloader nar, the LedgerOffloaderFactory#create has no param offloaderStats. Then it will throw an exception as follows:

2023-03-23T23:59:42,947 - ERROR - [main:PulsarService@918] - Failed to start Pulsar service: java.lang.AbstractMethodError: Receiver class io.streamnative.tieredstorage.pulsar.PulsarOffloaderFactory does not define or inherit an implementation of the resolved method 'abstract org.apache.bookkeeper.mledger.LedgerOffloader create(org.apache.pulsar.common.policies.data.OffloadPoliciesImpl, java.util.Map, org.apache.bookkeeper.common.util.OrderedScheduler, org.apache.bookkeeper.mledger.LedgerOffloaderStats)' of interface org.apache.bookkeeper.mledger.LedgerOffloaderFactory.
org.apache.pulsar.broker.PulsarServerException: java.lang.AbstractMethodError: Receiver class io.streamnative.tieredstorage.pulsar.PulsarOffloaderFactory does not define or inherit an implementation of the resolved method 'abstract org.apache.bookkeeper.mledger.LedgerOffloader create(org.apache.pulsar.common.policies.data.OffloadPoliciesImpl, java.util.Map, org.apache.bookkeeper.common.util.OrderedScheduler, org.apache.bookkeeper.mledger.LedgerOffloaderStats)' of interface org.apache.bookkeeper.mledger.LedgerOffloaderFactory.
	at org.apache.pulsar.broker.PulsarService.createManagedLedgerOffloader(PulsarService.java:1422) ~[classes/:?]
	at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:780) ~[classes/:?]
	at org.apache.pulsar.PulsarBrokerStarter$BrokerStarter.start(PulsarBrokerStarter.java:276) ~[classes/:?]
	at org.apache.pulsar.PulsarBrokerStarter.main(PulsarBrokerStarter.java:356) ~[classes/:?]

So we should make LedgerOffloaderFactory#create compatible with the previous version, and revert the origin signature to LedgerOffloaderFactory for compatibility.

After this PR, the user upgrades the managed-ledger dependency(LedgerOffloaderFactory is defined in managed-ledger). So the user offloader nar LedgerOffloaderFactory has both methods. One is with offloaderStats, another one is without offloaderStats.

If the pulsar is the old version, it loads the offloader nar, and using LedgerOffloaderFactory#create without offloadStats to create offloader. It works.

If the pulsar is the new version, it loads the offloader nar, and using LedgerOffloaderFactory#create with offloadStats to create the offloader. It also works.

Modifications

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

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:

poorbarcode · 2023-03-30T03:24:33Z

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

+             Map<String, String> userMetadata,
+             OrderedScheduler scheduler)
+            throws IOException {
+        return create(offloadPolicies, userMetadata, scheduler, null);


After this change, Is it possible to throw a NullPointerEx using the variable offloaderStats?

pulsar/tiered-storage/file-system/src/main/java/org/apache/bookkeeper/mledger/offload/filesystem/impl/FileSystemManagedLedgerOffloader.java

Lines 226 to 228 in 55523ac

long cost = System.nanoTime() - startReadTime;

this.offloaderStats.recordReadLedgerLatency(topicName, cost, TimeUnit.NANOSECONDS);

semaphore.acquire();

Such as line-227 above or other places.

Thanks for your reminder. Here we shouldn't make the method default. This method won't invoke in pulsar directly. In the pulsar master branch, it only invokes the method with the param offloaderStats. This method is compatible with the nar package, let the user implements by themselves.

horizonzy · 2023-03-31T02:08:24Z

@poorbarcode Could you help to review it again, thanks!

poorbarcode · 2023-03-31T05:42:21Z

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

             Map<String, String> userMetadata,
             OrderedScheduler scheduler,
             LedgerOffloaderStats offloaderStats)
        throws IOException;


Since this method T create(offloadPolicies, userMetadata, scheduler, offloaderStats), the changes in #13833 can not guarantee forward compatibility, right?

There are four scenarios:

old nar & old broker: I think it will work.

old nar & new broker: It will not work, and we do not guarantee forward compatibility. So it is okay.

new nar & old broker: The broker will call the method without @param offloaderStats. I think it will work.

we should make the method without @param offloaderStats non-default.

new nar & new broker I think it will work.

All things are good.

poorbarcode · 2023-04-01T11:17:25Z

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

             Map<String, String> userMetadata,
             OrderedScheduler scheduler,
             LedgerOffloaderStats offloaderStats)
        throws IOException;


There are four scenarios:

old nar & old broker: I think it will work.

old nar & new broker: It will not work, and we do not guarantee forward compatibility. So it is okay.

new nar & old broker: The broker will call the method without @param offloaderStats. I think it will work.

we should make the method without @param offloaderStats non-default.

new nar & new broker I think it will work.

All things are good.

hangc0276 · 2023-04-19T10:51:05Z

@codelipenghui @Technoboy- @poorbarcode I think this PR needs to be cherry-picked to branch-2.11 because it's for backporting the old interface.

After #13833, we change the signature of LedgerOffloaderFactory#create. But some users may use the old version LedgerOffloaderFactory in the offloader nar, when pulsar load the offloader nar, it will use the LedgerOffloaderFactory in the offloader nar, the LedgerOffloaderFactory#create has no param offloaderStats. Modifications: Make LedgerOffloaderFactory can load the old nar.

Technoboy- · 2023-05-09T06:25:40Z

@codelipenghui @Technoboy- @poorbarcode I think this PR needs to be cherry-picked to branch-2.11 because it's for backporting the old interface.

Done

Make LedgerOffloaderFactory compatible with old version.

764bb3f

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 23, 2023

horizonzy changed the title ~~Make LedgerOffloaderFactory compatible with old version.~~ [fix][broker]Make LedgerOffloaderFactory compatible with old version. Mar 24, 2023

horizonzy closed this Mar 24, 2023

horizonzy reopened this Mar 24, 2023

Technoboy- assigned horizonzy Mar 30, 2023

Technoboy- added this to the 3.0.0 milestone Mar 30, 2023

Technoboy- added area/broker ready-to-test labels Mar 30, 2023

Technoboy- closed this Mar 30, 2023

Technoboy- reopened this Mar 30, 2023

Technoboy- approved these changes Mar 30, 2023

View reviewed changes

poorbarcode reviewed Mar 30, 2023

View reviewed changes

Address the comments.

36bec9f

poorbarcode reviewed Mar 31, 2023

View reviewed changes

poorbarcode approved these changes Apr 1, 2023

View reviewed changes

poorbarcode changed the title ~~[fix][broker]Make LedgerOffloaderFactory compatible with old version.~~ [fix][broker]Make LedgerOffloaderFactory compatible with old nar. Apr 6, 2023

poorbarcode changed the title ~~[fix][broker]Make LedgerOffloaderFactory compatible with old nar.~~ [fix][broker]Make LedgerOffloaderFactory can load the old nar. Apr 6, 2023

poorbarcode merged commit 02c838c into apache:master Apr 10, 2023

hangc0276 added the release/2.11.2 label Apr 20, 2023

Technoboy- added the cherry-picked/branch-2.11 label May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker]Make LedgerOffloaderFactory can load the old nar. #19913

[fix][broker]Make LedgerOffloaderFactory can load the old nar. #19913

Uh oh!

horizonzy commented Mar 23, 2023 •

edited

Loading

Uh oh!

poorbarcode Mar 30, 2023 •

edited

Loading

Uh oh!

horizonzy Mar 30, 2023

Uh oh!

horizonzy commented Mar 31, 2023

Uh oh!

poorbarcode Mar 31, 2023

Uh oh!

horizonzy Mar 31, 2023

Uh oh!

poorbarcode Apr 1, 2023

Uh oh!

poorbarcode Apr 1, 2023

Uh oh!

hangc0276 commented Apr 19, 2023

Uh oh!

Technoboy- commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	long cost = System.nanoTime() - startReadTime;
	this.offloaderStats.recordReadLedgerLatency(topicName, cost, TimeUnit.NANOSECONDS);
	semaphore.acquire();

[fix][broker]Make LedgerOffloaderFactory can load the old nar. #19913

[fix][broker]Make LedgerOffloaderFactory can load the old nar. #19913

Uh oh!

Conversation

horizonzy commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

poorbarcode Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

horizonzy Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

horizonzy commented Mar 31, 2023

Uh oh!

poorbarcode Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

horizonzy Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

poorbarcode Apr 1, 2023

Choose a reason for hiding this comment

Uh oh!

poorbarcode Apr 1, 2023

Choose a reason for hiding this comment

Uh oh!

hangc0276 commented Apr 19, 2023

Uh oh!

Technoboy- commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

horizonzy commented Mar 23, 2023 •

edited

Loading

poorbarcode Mar 30, 2023 •

edited

Loading