You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "minwoo-jung (via GitHub)" <gi...@apache.org> on 2025/03/07 06:05:00 UTC
[PR] Fix HDFS as Deep Storage issue by adjusting dependencies in pom.xml [pinot]
minwoo-jung opened a new pull request, #15216:
URL: https://github.com/apache/pinot/pull/15216
For more details, please refer to issue [#15215](https://github.com/apache/pinot/issues/15215).
--
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
Re: [PR] Fix HDFS as Deep Storage issue by adjusting dependencies in pom.xml [pinot]
Posted by "minwoo-jung (via GitHub)" <gi...@apache.org>.
minwoo-jung commented on code in PR #15216:
URL: https://github.com/apache/pinot/pull/15216#discussion_r1984532112
##########
pom.xml:
##########
@@ -1375,8 +1375,8 @@
</dependency>
<dependency>
<groupId>org.apache.hadoop.thirdparty</groupId>
Review Comment:
@NihalJain Hello
I am not a Pinot developer, so I'm not entirely sure about the intention behind this. However, in the past, the HDFS library was not included directly in the package. But, in recent versions, I noticed that the library is included as a shaded dependency. So, it seems that the version management policy for the HDFS library has changed. It would be best for the responsible person to check whether this is the case or if we need to modify it to avoid including the library.
Additionally, depending on the policy regarding the HDFS library, the relevant guide documentation may also need to be updated, so I've asked the team to check that.
Furthermore, when using libraries that are sensitive to versions, these issues are quite common. However, I also know well that implementing it to work properly regardless of the library version is not easy. ^^
Enforcing a specific version of a library might be inconvenient when using compatible versions, but there are also advantages. For libraries sensitive to security and performance, enforcing a specific version is much better for stability.
If we do not enforce library versions, the usability would certainly increase, but it’s impossible to make the system work for all versions. So, personally, I try to enforce specific library versions, while documenting the compatible versions (e.g., in the README).
--
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
Re: [PR] Fix HDFS as Deep Storage issue by adjusting dependencies in pom.xml [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15216:
URL: https://github.com/apache/pinot/pull/15216#issuecomment-2705827530
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15216?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 63.45%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`6d11202`)](https://app.codecov.io/gh/apache/pinot/commit/6d112024dc6f7e03183d85b93b89d45f8509d1af?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1821 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15216 +/- ##
============================================
+ Coverage 61.75% 63.45% +1.70%
- Complexity 207 1367 +1160
============================================
Files 2436 2761 +325
Lines 133233 155600 +22367
Branches 20636 23928 +3292
============================================
+ Hits 82274 98743 +16469
- Misses 44911 49391 +4480
- Partials 6048 7466 +1418
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.45% <ø> (+1.83%)` | :arrow_up: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.44% <ø> (+1.69%)` | :arrow_up: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.42% <ø> (+35.69%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.45% <ø> (+1.70%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.45% <ø> (+1.71%)` | :arrow_up: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.04% <ø> (+9.15%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15216/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.00% <ø> (+6.27%)` | :arrow_up: |
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.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/15216?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>🚀 New features to boost your workflow: </summary>
- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- 📦 [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] Fix HDFS as Deep Storage issue by adjusting dependencies in pom.xml [pinot]
Posted by "minwoo-jung (via GitHub)" <gi...@apache.org>.
minwoo-jung commented on code in PR #15216:
URL: https://github.com/apache/pinot/pull/15216#discussion_r1984532112
##########
pom.xml:
##########
@@ -1375,8 +1375,8 @@
</dependency>
<dependency>
<groupId>org.apache.hadoop.thirdparty</groupId>
Review Comment:
Hello,
I am not a Pinot developer, so I'm not entirely sure about the intention behind this. However, in the past, the HDFS library was not included directly in the package. But, in recent versions, I noticed that the library is included as a shaded dependency. So, it seems that the version management policy for the HDFS library has changed. It would be best for the responsible person to check whether this is the case or if we need to modify it to avoid including the library.
Additionally, depending on the policy regarding the HDFS library, the relevant guide documentation may also need to be updated, so I've asked the team to check that.
Furthermore, when using libraries that are sensitive to versions, these issues are quite common. However, I also know well that implementing it to work properly regardless of the library version is not easy. ^^
Enforcing a specific version of a library might be inconvenient when using compatible versions, but there are also advantages. For libraries sensitive to security and performance, enforcing a specific version is much better for stability.
If we do not enforce library versions, the usability would certainly increase, but it’s impossible to make the system work for all versions. So, personally, I try to enforce specific library versions, while documenting the compatible versions (e.g., in the README).
--
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
Re: [PR] Fix HDFS as Deep Storage issue by adjusting dependencies in pom.xml [pinot]
Posted by "NihalJain (via GitHub)" <gi...@apache.org>.
NihalJain commented on code in PR #15216:
URL: https://github.com/apache/pinot/pull/15216#discussion_r1984513783
##########
pom.xml:
##########
@@ -1375,8 +1375,8 @@
</dependency>
<dependency>
<groupId>org.apache.hadoop.thirdparty</groupId>
Review Comment:
Not related to change but should not we let this come transitively from hadoop? I am not sure why we are version managing this. Is there a specific reason for doing so?
With current code, pinot cannot build (and then work smoothly) with multiple versions of hadoop. We will hit the same issue which @minwoo-jung today.
--
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
Re: [PR] Fix HDFS as Deep Storage issue by adjusting dependencies in pom.xml [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #15216:
URL: https://github.com/apache/pinot/pull/15216
--
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
Re: [PR] Fix HDFS as Deep Storage issue by adjusting dependencies in pom.xml [pinot]
Posted by "NihalJain (via GitHub)" <gi...@apache.org>.
NihalJain commented on code in PR #15216:
URL: https://github.com/apache/pinot/pull/15216#discussion_r1984513783
##########
pom.xml:
##########
@@ -1375,8 +1375,8 @@
</dependency>
<dependency>
<groupId>org.apache.hadoop.thirdparty</groupId>
Review Comment:
Not related to change but should not we let this come transitively from hadoop? I am not sure why we are version managing this. Is there a specific reason for doing so?
With current code, pinot cannot build (and then work smoothly) with multiple versions of hadoop. We will hit the same issue which @minwoo-jung is facing today.
--
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