apache / distributedlog

Apache DistributedLog
Apache License 2.0
185 stars 74 forks source link

Issue-193: Shade Bookkeeper and unshade ZooKeeper in core module #194

Closed ArvinDevel closed 6 years ago

ArvinDevel commented 6 years ago

Descriptions of the changes in this PR:

Shade Bookkeeper and unshade ZooKeeper in core module


Be sure to do all of the following to help us incorporate your contribution quickly and easily:

  • [ ] Make sure the PR title is formatted like: <Issue-# or DL-#>: Description of pull request e.g. Issue-123: Description ... e.g. DL-1234: Description ...
  • [ ] Make sure tests pass via mvn clean apache-rat:check install findbugs:check.
  • [ ] Replace '#' with in <Issue-# or DL-#> in the title with the actual Issue/JIRA number.

ArvinDevel commented 6 years ago

Yes, I know that, if you want to use the specific bk included in dlog, and there exists the bk conflict problem, you will need the shaded class, otherwise you don't need to . Usually we don't use the shaded jar because dlog already provides a well-defined api. zk as a coordinating service, it's used widely, unshade it may has some benefits. Sorry for that I don't understand "in fact in zk there are configuration options wich represent class names".

eolivelli commented 6 years ago

Like the socket connection factory, like if you want to switch to the netty based implementation you have to set a property, I don't have links, I am using my phone at the moment, sorry

zhaijack commented 6 years ago

@ArvinDevel , Would you please provide more details for the reason of this change? Regarding @eolivelli 's question, I think there may be some similar configurations, which reference a class name, in zk as bk, like this style in bk_server.conf:

ledgerStorageClass=org.apache.bookkeeper.bookie.SortedLedgerStorage
ArvinDevel commented 6 years ago

The reason is that I needed a shaded jar in a project where the bk version has conflicts, and the existed shaded jar is not competent. I think this is a promotion to #161. I think shade bk is necessary, but I'm not sure whether the unshading to zk is deserve(this change may only necessary in my project). @zhaijack

sijie commented 6 years ago

@ArvinDevel I am wondering - instead of changing current shade rules, can you create a new shade execution "bkshade" (similar as this one https://github.com/apache/distributedlog/blob/master/distributedlog-core/pom.xml#L200), so we have jar that includes shading "bk".

ArvinDevel commented 6 years ago

@sijie This is a good idea, and I updated it.

ArvinDevel commented 6 years ago

Now I have a question, is this PR qualified? Due to my specific use case, I provided a shade execution which is not common enough, eg. unshading zk.

sijie commented 6 years ago

@ArvinDevel can we include shading other packages as well? basically copying the existing shade rules and include shading bk? that would be a better approach?

ArvinDevel commented 6 years ago

At first I just include bk, but found that shaded jar can't work. if another package is dependency of bk, and is dependency in your project too, then this package is necessary. This question involves at least netty and common package in my project, so I copied the common used package from the existing "shaded-all" part. @sijie

sijie commented 6 years ago

gotcha. lgtm +1