You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "KKcorps (via GitHub)" <gi...@apache.org> on 2023/08/03 14:21:45 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #11264: Remove JsonUtils dependency due to classpath issues
KKcorps opened a new pull request, #11264:
URL: https://github.com/apache/pinot/pull/11264
(no comment)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] [pinot] snleee merged pull request #11264: Remove JsonUtils dependency due to classpath issues
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee merged PR #11264:
URL: https://github.com/apache/pinot/pull/11264
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] [pinot] snleee commented on pull request #11264: Remove JsonUtils dependency due to classpath issues
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on PR #11264:
URL: https://github.com/apache/pinot/pull/11264#issuecomment-1664953653
This is more for short term fix. We can revert this after we resolve the shading issue. Ideally, we should have a single definition of `JsonUtils` but it looks that it not easy to achieve. Let's discuss on this.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] [pinot] Jackie-Jiang commented on pull request #11264: Remove JsonUtils dependency due to classpath issues
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11264:
URL: https://github.com/apache/pinot/pull/11264#issuecomment-1664932728
@snleee @KKcorps `JsonUtils` is a pinot util class. How is it possible to pull multiple versions of it?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] [pinot] snleee commented on pull request #11264: Remove JsonUtils dependency due to classpath issues
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on PR #11264:
URL: https://github.com/apache/pinot/pull/11264#issuecomment-1664611474
Adding more context on this PR:
- We have seen that multiple versions of `JsonUtils` are pulled. (While having the same name space, the parameter's class namespace are different due to shading).
- This is a short/mid term fix where we remove `JsonUtils` dependency from Kinesis library to resolve the dependency conflict.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] [pinot] codecov-commenter commented on pull request #11264: Remove JsonUtils dependency due to classpath issues
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11264:
URL: https://github.com/apache/pinot/pull/11264#issuecomment-1664167050
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11264?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11264](https://app.codecov.io/gh/apache/pinot/pull/11264?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f710cfd) into [master](https://app.codecov.io/gh/apache/pinot/commit/49fe4f2c6eeddd64161bc4d9c9dfe5fdb0c34d89?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (49fe4f2) will **decrease** coverage by `0.01%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11264 +/- ##
==========================================
- Coverage 0.11% 0.11% -0.01%
==========================================
Files 2229 2229
Lines 119959 119964 +5
Branches 18173 18173
==========================================
Hits 137 137
- Misses 119802 119807 +5
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `0.00% <0.00%> (ø)` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (ø)` | |
| unittests2temurin17 | `0.11% <0.00%> (ø)` | |
| unittests2temurin20 | `0.11% <0.00%> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/pinot/pull/11264?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...in/stream/kinesis/KinesisPartitionGroupOffset.java](https://app.codecov.io/gh/apache/pinot/pull/11264?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcGx1Z2lucy9waW5vdC1zdHJlYW0taW5nZXN0aW9uL3Bpbm90LWtpbmVzaXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9zdHJlYW0va2luZXNpcy9LaW5lc2lzUGFydGl0aW9uR3JvdXBPZmZzZXQuamF2YQ==) | `0.00% <0.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] [pinot] Jackie-Jiang commented on pull request #11264: Remove JsonUtils dependency due to classpath issues
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11264:
URL: https://github.com/apache/pinot/pull/11264#issuecomment-1664938275
@xiangfu0 I'm not very familiar with shading, but I don't think this is the correct fix because basically this means we cannot use any utils that pulls external dependencies. Can you also provide your opinion?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] [pinot] gortiz commented on pull request #11264: Remove JsonUtils dependency due to classpath issues
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #11264:
URL: https://github.com/apache/pinot/pull/11264#issuecomment-1667415680
There are two different issues:
- We have JsonUtils several times in the classpath with the same package. Luckily, the correct one is loaded first.
- The plugins shade jackson using their package prefix. That means that when they call public spi that use jackson, they use their own package. For example, they call `<apache pinot package>.JsonUtils.stringToObject(String, <their-package-prefix>.TypeReference)`, which does not exist.
That later problem is the one that affected us. The problem here is that by adding jackson arguments and results to our spi methods, we are indicating that jackson should not be shaded, when in fact jackson is one the main package we want to shade due to incompatibilities in the classpath.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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] [pinot] Jackie-Jiang commented on pull request #11264: Remove JsonUtils dependency due to classpath issues
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11264:
URL: https://github.com/apache/pinot/pull/11264#issuecomment-1664934031
Does that mean we cannot use any class under `pinot-spi`?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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