Skip to content

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

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

Merged
poorbarcode merged 2 commits into from
Apr 10, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
764bb3f
Make LedgerOffloaderFactory compatible with old version.
horizonzy Mar 23, 2023
36bec9f
Address the comments.
horizonzy Mar 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion managed-ledger/src/main/java/org/apache/bookkeeper/mledger/LedgerOffloaderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,20 +42,37 @@ public interface LedgerOffloaderFactory<T extends LedgerOffloader> {
boolean isDriverSupported(String driverName);

/**
* Create a ledger offloader with the provided configuration, user-metadata and scheduler.
* Create a ledger offloader with the provided configuration, user-metadata, scheduler and offloaderStats.
*
* @param offloadPolicies offload policies
* @param userMetadata user metadata
* @param scheduler scheduler
* @return the offloader instance
* @throws IOException when fail to create an offloader
*/
T create(OffloadPoliciesImpl offloadPolicies,
Map<String, String> userMetadata,
OrderedScheduler scheduler)
throws IOException;


/**
* Create a ledger offloader with the provided configuration, user-metadata, scheduler and offloaderStats.
*
* @param offloadPolicies offload policies
* @param userMetadata user metadata
* @param scheduler scheduler
* @param offloaderStats offloaderStats
* @return the offloader instance
* @throws IOException when fail to create an offloader
*/
T create(OffloadPoliciesImpl offloadPolicies,
Map<String, String> userMetadata,
OrderedScheduler scheduler,
LedgerOffloaderStats offloaderStats)
throws IOException;
Copy link
Contributor

@poorbarcode poorbarcode Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@horizonzy horizonzy Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Contributor

@poorbarcode poorbarcode Apr 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



/**
* Create a ledger offloader with the provided configuration, user-metadata, schema storage and scheduler.
*
Expand All @@ -66,6 +83,26 @@ T create(OffloadPoliciesImpl offloadPolicies,
* @return the offloader instance
* @throws IOException when fail to create an offloader
*/
default T create(OffloadPoliciesImpl offloadPolicies,
Map<String, String> userMetadata,
SchemaStorage schemaStorage,
OrderedScheduler scheduler)
throws IOException {
return create(offloadPolicies, userMetadata, scheduler);
}

/**
* Create a ledger offloader with the provided configuration, user-metadata, schema storage,
* scheduler and offloaderStats.
*
* @param offloadPolicies offload policies
* @param userMetadata user metadata
* @param schemaStorage used for schema lookup in offloader
* @param scheduler scheduler
* @param offloaderStats offloaderStats
* @return the offloader instance
* @throws IOException when fail to create an offloader
*/
default T create(OffloadPoliciesImpl offloadPolicies,
Map<String, String> userMetadata,
SchemaStorage schemaStorage,
Expand Down
4 changes: 2 additions & 2 deletions managed-ledger/src/main/java/org/apache/bookkeeper/mledger/LedgerOffloaderStatsDisable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@

import java.util.concurrent.TimeUnit;

class LedgerOffloaderStatsDisable implements LedgerOffloaderStats {
public class LedgerOffloaderStatsDisable implements LedgerOffloaderStats {

static final LedgerOffloaderStats INSTANCE = new LedgerOffloaderStatsDisable();
public static final LedgerOffloaderStats INSTANCE = new LedgerOffloaderStatsDisable();

private LedgerOffloaderStatsDisable() {

Expand Down
8 changes: 8 additions & 0 deletions ...va/org/apache/bookkeeper/mledger/offload/filesystem/FileSystemLedgerOffloaderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.bookkeeper.common.util.OrderedScheduler;
import org.apache.bookkeeper.mledger.LedgerOffloaderFactory;
import org.apache.bookkeeper.mledger.LedgerOffloaderStats;
import org.apache.bookkeeper.mledger.LedgerOffloaderStatsDisable;
import org.apache.bookkeeper.mledger.offload.filesystem.impl.FileSystemManagedLedgerOffloader;
import org.apache.pulsar.common.policies.data.OffloadPoliciesImpl;

Expand All @@ -32,6 +33,13 @@ public boolean isDriverSupported(String driverName) {
return FileSystemManagedLedgerOffloader.driverSupported(driverName);
}

@Override
public FileSystemManagedLedgerOffloader create(OffloadPoliciesImpl offloadPolicies,
Map<String, String> userMetadata, OrderedScheduler scheduler)
throws IOException {
return create(offloadPolicies, userMetadata, scheduler, LedgerOffloaderStatsDisable.INSTANCE);
}

@Override
public FileSystemManagedLedgerOffloader create(OffloadPoliciesImpl offloadPolicies,
Map<String, String> userMetadata,
Expand Down
7 changes: 7 additions & 0 deletions .../main/java/org/apache/bookkeeper/mledger/offload/jcloud/JCloudLedgerOffloaderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.bookkeeper.common.util.OrderedScheduler;
import org.apache.bookkeeper.mledger.LedgerOffloaderFactory;
import org.apache.bookkeeper.mledger.LedgerOffloaderStats;
import org.apache.bookkeeper.mledger.LedgerOffloaderStatsDisable;
import org.apache.bookkeeper.mledger.offload.jcloud.impl.BlobStoreManagedLedgerOffloader;
import org.apache.bookkeeper.mledger.offload.jcloud.provider.JCloudBlobStoreProvider;
import org.apache.bookkeeper.mledger.offload.jcloud.provider.TieredStorageConfiguration;
Expand All @@ -44,6 +45,12 @@ public boolean isDriverSupported(String driverName) {
return JCloudBlobStoreProvider.driverSupported(driverName);
}

@Override
public BlobStoreManagedLedgerOffloader create(OffloadPoliciesImpl offloadPolicies, Map<String, String> userMetadata,
OrderedScheduler scheduler) throws IOException {
return create(offloadPolicies, userMetadata, scheduler, LedgerOffloaderStatsDisable.INSTANCE);
}

@Override
public BlobStoreManagedLedgerOffloader create(OffloadPoliciesImpl offloadPolicies, Map<String, String> userMetadata,
OrderedScheduler scheduler,
Expand Down