You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/03/24 23:32:23 UTC
[GitHub] [incubator-pinot] amrishlal opened a new pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
amrishlal opened a new pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716
## Description
Pinot is using a very old version of Avro (avro-1.7.6). LinkedIn data team is asking us to migrate to a more recent version of Avro to maintain better better binary compatibility with other tools using Avro. This PR upgrades Pinot to avro-1.9.2. This also involves upgrading to Parquet version 1.11.1 since Parquet also has dependency on Avro. All other changes in this PR are for resolving transitive dependencies.
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.
If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text
## Documentation
If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#discussion_r601698987
##########
File path: pom.xml
##########
@@ -806,6 +820,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
Review comment:
E.g. some of the transitive dependencies of `hadoop-client` got excluded in root pom file, such as `jersey-core`, `protobuf-java`, etc. Make sure those transitive dependencies won't get pulled in again by `hadoop-mapreduce-client-core`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#discussion_r601694003
##########
File path: pom.xml
##########
@@ -806,6 +820,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
Review comment:
@jackjlli Not sure if I understand, what you are saying, completely, but all the dependencies (direct and transitive) are converging with these changes. These are the minimal set of changes that we need to do to upgrade to Avro 1.9.2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] FelixGV commented on pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
FelixGV commented on pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#issuecomment-808856352
Hi,
Have you considered leveraging the [avro-util](https://github.com/linkedin/avro-util) library? There are two parts to this project that could be relevant to Pinot:
1. The [helper](https://github.com/linkedin/avro-util/tree/master/helper) module provides a version-agnostic shim for dealing with Avro. Using this shim would allow Pinot to work with any version of Avro between 1.4 and 1.10, even though the Apache Avro project introduced many breaking API changes throughout these versions. Whichever version is found on the classpath will be dealt transparently.
2. The [avro-fastserde](https://github.com/linkedin/avro-util/tree/master/avro-fastserde) module is an alternative implementation of Avro serde which does runtime code generation to create an optimized serde for each schema reader/writer pair. It is more CPU-efficient and in some cases more GC-efficient as well. We've noticed 2-10x serde performance increase with it, depending on schemas. This module itself leverages the aforementioned helper module in order to achieve compatibility with Avro 1.4-1.10.
Let me know if you have any questions.
-F
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] FelixGV edited a comment on pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
FelixGV edited a comment on pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#issuecomment-808856352
Hi,
Have you considered leveraging the [avro-util](https://github.com/linkedin/avro-util) library? There are two parts to this project that could be relevant to Pinot:
1. The [helper](https://github.com/linkedin/avro-util/tree/master/helper) module provides a version-agnostic shim for dealing with Avro. Using this shim would allow Pinot to work with any version of Avro between 1.4 and 1.10, even though the Apache Avro project introduced many breaking API changes throughout these versions. Whichever version is found on the classpath will be dealt with transparently.
2. The [avro-fastserde](https://github.com/linkedin/avro-util/tree/master/avro-fastserde) module is an alternative implementation of Avro serde which does runtime code generation to create an optimized serde for each schema reader/writer pair. It is more CPU-efficient and in some cases more GC-efficient as well. We've noticed 2-10x serde performance increase with it, depending on schemas. This module itself leverages the aforementioned helper module in order to achieve compatibility with Avro 1.4-1.10.
Let me know if you have any questions.
-F
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#issuecomment-806373357
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6716?src=pr&el=h1) Report
> Merging [#6716](https://codecov.io/gh/apache/incubator-pinot/pull/6716?src=pr&el=desc) (d464f88) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `23.12%`.
> The diff coverage is `42.93%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6716?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6716 +/- ##
===========================================
- Coverage 66.44% 43.32% -23.13%
===========================================
Files 1075 1396 +321
Lines 54773 67765 +12992
Branches 8168 9807 +1639
===========================================
- Hits 36396 29361 -7035
- Misses 15700 35947 +20247
+ Partials 2677 2457 -220
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.32% <42.93%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6716?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...t/broker/broker/BasicAuthAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0Jhc2ljQXV0aEFjY2Vzc0NvbnRyb2xGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| ... and [1424 more](https://codecov.io/gh/apache/incubator-pinot/pull/6716/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6716?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6716?src=pr&el=footer). Last update [3c055f3...d464f88](https://codecov.io/gh/apache/incubator-pinot/pull/6716?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#discussion_r601089974
##########
File path: pinot-common/pom.xml
##########
@@ -281,6 +282,11 @@
<groupId>io.grpc</groupId>
<artifactId>grpc-stub</artifactId>
</dependency>
+ <dependency>
Review comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#discussion_r601763718
##########
File path: pom.xml
##########
@@ -806,6 +820,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
Review comment:
Some of the exclusions that we have in pom files may be redundant :-) But it either case, there are no conflicting dependencies being pulled in because of hadoop-mapreduce-client-core. If a higher-level project includes us and runs into conflicts because of our transitive dependencies (say dependencies being pulled in by the hadoop libraries that we are pulling in) they can always use `<exclude>`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#discussion_r601763718
##########
File path: pom.xml
##########
@@ -806,6 +820,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
Review comment:
Some of the exclusions that we have in pom files may be redundant :-) But it either case, there are no conflicting dependencies being pulled in because of hadoop-mapreduce-client-core. If a higher-level project includes us and runs into conflicts because of our transitive dependencies (say dependencies being pulled in by the hadoop libraries that we are pulling in) they can always use <exclude>.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#discussion_r600956964
##########
File path: pinot-common/pom.xml
##########
@@ -281,6 +282,11 @@
<groupId>io.grpc</groupId>
<artifactId>grpc-stub</artifactId>
</dependency>
+ <dependency>
Review comment:
I think versioned dependencies should go to root pom, here you just need to refer to that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#discussion_r601763718
##########
File path: pom.xml
##########
@@ -806,6 +820,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
Review comment:
Some of the exclusions that we have in pom files may be redundant :-) But it either case, there are no conflicting dependencies being pulled in because of hadoop-mapreduce-client-core. If a higher-level project includes us and runs into conflicts because of our transitive dependencies (say dependencies being pulled in by the hadoop libraries that we are pulling in) they can always use `<exclusion>`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli merged pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
jackjlli merged pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6716: Upgrade to avro-1.92 and resolve transitive dependencies.
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6716:
URL: https://github.com/apache/incubator-pinot/pull/6716#discussion_r601673127
##########
File path: pom.xml
##########
@@ -806,6 +820,12 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
Review comment:
I notice there are some excluded transitive dependencies in other hadoop related dependencies. Please make sure this one also does the same thing for those transitive dependencies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org