Move all prestodb dependencies into a separated module #8266
Conversation
xiangfu0
force-pushed
the
pinot-jdk8-modules
branch
3 times, most recently
from
ece9801 to
d2c7fd5
Compare
xiangfu0
force-pushed
the
pinot-jdk8-modules
branch
from
d2c7fd5 to
96571d1
Compare
xiangfu0
requested review from
Jackie-Jiang,
jackjlli,
mcvsubbu,
richardstartin and
snleee
pinot-common/pom.xml
Outdated
There was a problem hiding this comment.
The final goal is to remove the JDK8 build profile and move that to the presto dependency side :p
There was a problem hiding this comment.
Since the source code are the same, do we need to add these new modules?
There was a problem hiding this comment.
Yes, Presto side will pull and compile the dependencies transitively, and those jars have to be JDK 8 binary code.
There was a problem hiding this comment.
Does this still require code to be java 8 compatible?
There was a problem hiding this comment.
Here are some leftover works once folks(e.g. Uber) upgrade their build system from JDK 8. We can completely change the profile of the JDK8 build and call that Pinot deprecated JDK 8 build but can be released with JDK 8 binary.
xiangfu0
force-pushed
the
pinot-jdk8-modules
branch
from
e901c08 to
34e52b9
Compare
xiangfu0
force-pushed
the
pinot-jdk8-modules
branch
from
8d604c9 to
57a63fa
Compare
Codecov Report
@@ Coverage Diff @@
## master #8266 +/- ##
============================================
- Coverage 70.78% 64.05% -6.73%
+ Complexity 4243 4240 -3
============================================
Files 1631 1586 -45
Lines 85281 83567 -1714
Branches 12845 12674 -171
============================================
- Hits 60364 53532 -6832
- Misses 20746 26173 +5427
+ Partials 4171 3862 -309
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
LGTM.
It would be good if we can extract the common dependencies for jdk11 and jdk8 pom file so that we don't need to manually keep them in-sync
jackjlli
left a comment
jackjlli
left a comment
There was a problem hiding this comment.
Currently Pinot at LinkedIn is partly on jdk 8, mainly on the offline ingestion part (very similar to the situation of Presto). And bumping up jdk version to 11 in the offline world is non-trivial since there are a lot dependencies/components that need to be Java 11 compatible (e.g. Hadoop 3.0+ needs to be used, etc).
I also tried out the Pinot Java 11 compiled code in current offline env but since Java 8 is being used there, there is no way to run the Pinot Java 11 compiled code there (it's not allowed to run code compiled in higher jdk version in a lower jvm version).
What's the plan for these jdk8 modules? Will they be left in Presto? If that's the case, we may also need some extra modules to be jdk 8 compatible.
| <phase.prop>package</phase.prop> | ||
| </properties> | ||
|
|
||
| <profiles> |
There was a problem hiding this comment.
I don't see any new module for these pluggable modules. Will they come in the following PRs or is there any other plan?
There was a problem hiding this comment.
no plan, we just don't have to release those modules in jdk8 really.
There was a problem hiding this comment.
presto-pinot-driver has transitive test dependencies on those jars, so we actually don't need to release those jars with JDK 8.
There was a problem hiding this comment.
But we do have some code which is maintained by Pinot only. E.g. in pinot-avro-base module, we have AvroRecordReader which is only for Pinot. How do we deal with this code?
There was a problem hiding this comment.
right, so for any dependencies that presto is depending on, we need to make those have a jdk8 version.
For module like pinot-avro-base, it's not required the compile dependencies of presto-pinot-driver but only test dependency for pinot-core, so this time I don't include them.
There was a problem hiding this comment.
But we do have some code which is maintained by Pinot only. E.g. in
pinot-avro-basemodule, we haveAvroRecordReaderwhich is only for Pinot. How do we deal with this code?
We don't need to do anything with this code.
For LinkedIn internal, you can always use the parameter to build JDK 8 binary code jars and publish them to internal artifactory. This change impacts maven that's why we have to separate the modules. |
Yes I understand the purpose of this PR, but what'd be the next steps? After splitting out to the separate modules, will the existing modules like |
So there are two ways to generate JDK 8 binary:
For 1, you need to make sure the source code is JDK 8 compatible |
For pinot-avro etc, so far I don't find the need for moving to publish jdk8 jars. |
5 participants
Description
The goal here is to free pinot modules from JDK 8 binding for releases.
The problem of Pinot is still partially depends on JDK 8 is due to PrestoDB project still on JDK8 and PrestoDB requiring all transitive dependencies compilable with JDK8.
Solution:
pinot-spi,pinot-segment-spi,pinot-segment-local,pinot-common,pinot-coreand use symlink for src directorypresto-driverto build the above jars in JDK8presto-pinot-driveronly depends on those jdk8 modules and built with the profile flag.How to build:
Note(My hope as well):
Once PrestoDB upgraded JDK, we can retire those jdk8 modules.
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notesand complete the section on Release Notes)Release Notes
Documentation