[PIP-50] Package management implementation (part-1)#5613

Closed

zymap wants to merge 5 commits intoapache:masterfrom

zymap:pkg_management

Member

zymap commented Nov 11, 2019

Motivation

Implementation of package management.
https://github.com/apache/pulsar/wiki/PIP-50%3A-Package-Management

Modifications

Implement package management
Abstraction storage

zymap added 2 commits

November 11, 2019 17:29


          [PIP-50] Package management implementation (part-1)

1e83e4d

---

*Motivation*

Implementation of package management.
https://github.com/apache/pulsar/wiki/PIP-50%3A-Package-Management

*Modifications*

- Implement package management
- Abstraction storage


          Revert changes

9de61ed

Member Author

zymap commented Nov 11, 2019

@sijie @jiazhai @wolfstudy PTAL

Member Author

zymap commented Nov 11, 2019

run java8 tests

sijie requested changes

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

pulsar-package-management/pom.xml Outdated Show resolved Hide resolved

pulsar-package-management/src/main/java/pkg/management/PackageStorageProvider.java Outdated Show resolved Hide resolved

pulsar-package-management/src/main/java/pkg/management/PkgMetadata.java Outdated Show resolved Hide resolved

pulsar-package-management/src/main/java/pkg/management/exception/PackageNotFoundException.java Outdated Show resolved Hide resolved

pulsar-package-management/src/main/java/pkg/management/storage/bk/BKStorage.java Outdated Show resolved Hide resolved

pulsar-package-management/src/main/java/pkg/management/storage/bk/BKStorage.java Outdated Show resolved Hide resolved

pulsar-package-management/src/main/java/pkg/management/storage/bk/BKStorage.java Outdated Show resolved Hide resolved

pulsar-package-management/src/main/java/pkg/management/storage/bk/BKStorage.java Outdated Show resolved Hide resolved

pulsar-package-management/src/main/java/pkg/management/storage/bk/BKStorage.java Outdated Show resolved Hide resolved

sijie assigned zymap

sijie added this to the 2.5.0 milestone

zymap force-pushed the pkg_management branch 2 times, most recently from ac36499 to d8c6ce8 Compare

November 12, 2019 08:18


          Address comments

2dca4a2

zymap force-pushed the pkg_management branch from d8c6ce8 to 2dca4a2 Compare

November 13, 2019 02:33


          Make read and write distributed log async

30232be

zymap force-pushed the pkg_management branch from abb5121 to 30232be Compare

November 20, 2019 05:54

Member Author

zymap commented Nov 21, 2019

@sijie PTAL. Thanks.

sijie modified the milestones: 2.5.0, 2.6.0

sijie added area/broker type/enhancement labels

sijie reviewed

View reviewed changes

Member

sijie left a comment

Please create an issue to track adding documentation about this new package management system.

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/PackageStorage.java Outdated Show resolved Hide resolved

...package-manager/src/main/java/org/apache/pulsar/packages/manager/PackageStorageProvider.java Outdated Show resolved Hide resolved

...ain/java/org/apache/pulsar/packages/manager/exception/PackageMetaAlreadyExistsException.java

+              /**
+               * Thrown package metadata already existAsync exceptions.
+               */
+              public class PackageMetaAlreadyExistsException extends PackageException {

Member

sijie Nov 25, 2019

why do we have two AlreadyExistsException? Does it really happen that the package exists but the metadata doesn't exist?

Member Author

zymap Dec 26, 2019

Because the metadata and package are two files, delete a package that needs to delete metadata as well. Delete package and metadata are two operations and I can not promise after deleting package the metadata will delete successfully.
If one of the delete operations was failed and the user ignores the error and uploads the same package again, the metadata or the package may be already existent.
Do you have some advice?

...src/main/java/org/apache/pulsar/packages/manager/exception/PackageMetaNotFoundException.java

+              /**
+               * Thrown package metadata not found exceptions.
+               */
+              public class PackageMetaNotFoundException extends PackageException {

Member

sijie Nov 25, 2019

Same comments similar to AlreadyExistsException.

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/impl/PackageImpl.java Outdated Show resolved Hide resolved

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/impl/PackageImpl.java Outdated

+                      return packageStorage.existAsync(metadataPath)
+                          .thenCompose(metaExists -> metaExists
+                              ? packageStorage.deleteAsync(metadataPath)
+                              : FutureUtil.failedFuture(new PackageMetaNotFoundException("Package metadata does not exists")))

Member

sijie Nov 25, 2019

data can still exists when the metadata file doesn't exist. e.g. a delete operation failed in between deleting metadat and data.

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/impl/PackageImpl.java Outdated Show resolved Hide resolved

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/impl/PackageImpl.java Outdated

+                          .thenApply(names -> names.stream().map(PackageName::get).collect(Collectors.toList()));
+                  }
+                  private String getMetadataStoragePath(PackageName packageName) {

Member

sijie Nov 25, 2019

static

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/impl/PackageImpl.java Outdated Show resolved Hide resolved

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/impl/PackageImpl.java Outdated Show resolved Hide resolved

sijie requested changes

View reviewed changes

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/naming/PackageName.java

+                      return get(pkgName);
+                  }
+                  public static PackageName get(String type, String name, String version) {

Member

sijie Nov 25, 2019

don't you need to ensure the name is a validate "topic" name?

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/naming/PackageName.java Outdated

+                      try {
+                          return cache.get(packageName);
+                      } catch (ExecutionException e) {
+                          throw (RuntimeException) e.getCause();

Member

sijie Nov 25, 2019

Are you sure it is a safe cast to RuntimeException?

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/naming/PackageName.java

+                      this.namespaceName = NamespaceName.get(parts.get(0), parts.get(1));
+                      rest = parts.get(2);
+                      parts = Splitter.on("@").splitToList(rest);

Member

sijie Nov 25, 2019

don't you need to check the length first? e.g. if people don't specify the version "@".

Also if people don't specify the version, should we make the default version to latest?

pulsar-package-manager/src/main/java/org/apache/pulsar/packages/manager/naming/PackageType.java Outdated

+                              return e;
+                          }
+                      }
+                      throw new IllegalArgumentException("Invalid topic domain: '" + value + "'");

Member

sijie Nov 25, 2019

why this is "topic domain"?

...ge-manager/src/main/java/org/apache/pulsar/packages/manager/storage/bk/BKPackageStorage.java Outdated

+                      CompletableFuture<Boolean> future = new CompletableFuture<>();
+                      CompletableFuture.runAsync(() -> {
+                          try {
+                              future.complete(namespace.logExists(path));

Member

sijie Nov 25, 2019

namespace.getNamespaceDriver().getLogMetadataStore().getLogLocation() would return a completable future.

Member Author

zymap Dec 26, 2019

getLogLocation() seems not the log path. It's a URI. I find the namespace.getNamespaceDriver().getLogStreamMetadataStore(NamespaceDriver.Role.WRITER).logExists(uriOptional.get(), path) can reture a completable future.

...kage-manager/src/main/java/org/apache/pulsar/packages/manager/storage/bk/DLOutputStream.java Outdated

+                      List<CompletableFuture<DLOutputStream>> writeFuture = new ArrayList<>();
+                      byte[] bytes = new byte[1024];
+                      try {
+                          while (inputStream.read(bytes) != -1) {

Member

sijie Nov 25, 2019

inputStream.read is a synchronous operation. you have to call this in a separate thread to not blocking the thread calling writeAsync.

...-package-manager/src/test/java/org/apache/pulsar/packages/manager/impl/PackagesImplTest.java Outdated

+              import org.testng.annotations.BeforeMethod;
+              import org.testng.annotations.Test;
+              public class PackagesImplTest {

Member

sijie Nov 25, 2019

The test needs to cover different test cases.

...ckage-manager/src/test/java/org/apache/pulsar/packages/manager/storage/bk/BKStorageTest.java Outdated

+              import org.testng.annotations.BeforeMethod;
+              import org.testng.annotations.Test;
+              public class BKStorageTest {

Member

sijie Nov 25, 2019

Rename this to BKStorageMockTest and add one test for interacting with a real BK cluster.

...e-manager/src/test/java/org/apache/pulsar/packages/manager/storage/bk/DLInputStreamTest.java

+              /**
+               * Unit test of {@link DLInputStream}.
+               */
+              public class DLInputStreamTest {

Member

sijie Nov 25, 2019

Please all add test cases for error cases.

...-manager/src/test/java/org/apache/pulsar/packages/manager/storage/bk/DLOutputStreamTest.java

+              /**
+               * Unit test of {@link DLOutputStream}.
+               */
+              public class DLOutputStreamTest {

Member

sijie Nov 25, 2019

Please all add test cases for error cases.


          Address comments

c656f2a

zymap force-pushed the pkg_management branch from 063b0a7 to c656f2a Compare

December 26, 2019 14:39

Member Author

zymap commented Dec 30, 2019

@sijie I can not retrigger the test. So please take a look when you have time.

Contributor

codelipenghui commented May 19, 2020

@zymap Could you please rebase with the master branch? @sijie Shall we need to onboard this PR in 2.6.0 release?

sijie mentioned this pull request

[discussion] Pulsar release 2.6.0 #5819

Closed

codelipenghui modified the milestones: 2.6.0, 2.7.0

codelipenghui modified the milestones: 2.7.0, 2.8.0

zymap closed this

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

Labels

area/broker type/enhancement