Skip to content

mark spark-launcher as a provided dependency#9766

Closed
jadami10 wants to merge 1 commit intoapache:masterfrom
jadami10:jadami/spark-launcher-provided-upstream
Closed

mark spark-launcher as a provided dependency#9766
jadami10 wants to merge 1 commit intoapache:masterfrom
jadami10:jadami/spark-launcher-provided-upstream

Conversation

@jadami10
Copy link
Contributor

@jadami10 jadami10 commented Nov 9, 2022

I'm hoping someone that understands this code path well can review. But this marks the spark_launcher jar to be provided. We ran into this in a really weird case:

  • we are in the midst of a spark2 -> spark3 migration
  • we fork spark, and we reverted a change to the launcher on our spark fork to work with the old launcher sh files
  • we build pinot with shaded jars, i believe to pull in a correct, later hadoop dependency

Because this jar gets shaded, we were using the later code that didn't work with our launcher shell script. Spark core is already scoped as provided, so this seems like the desired behavior here, too.

@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2022

Codecov Report

Merging #9766 (76c76b6) into master (c48926a) will decrease coverage by 6.09%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #9766      +/-   ##
============================================
- Coverage     70.11%   64.02%   -6.10%     
- Complexity     4977     5356     +379     
============================================
  Files          1955     1902      -53     
  Lines        104851   102438    -2413     
  Branches      15874    15587     -287     
============================================
- Hits          73518    65585    -7933     
- Misses        26190    32085    +5895     
+ Partials       5143     4768     -375     
Flag Coverage Δ
integration1 ?
integration2 ?
unittests1 67.62% <ø> (+<0.01%) ⬆️
unittests2 15.66% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...va/org/apache/pinot/common/config/NettyConfig.java 0.00% <0.00%> (-100.00%) ⬇️
...a/org/apache/pinot/common/metrics/MinionMeter.java 0.00% <0.00%> (-100.00%) ⬇️
...g/apache/pinot/common/metrics/ControllerMeter.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/BrokerQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/MinionQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
...ache/pinot/server/access/AccessControlFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...he/pinot/common/messages/SegmentReloadMessage.java 0.00% <0.00%> (-100.00%) ⬇️
...he/pinot/common/messages/TableDeletionMessage.java 0.00% <0.00%> (-100.00%) ⬇️
...pinot/core/data/manager/realtime/TimerService.java 0.00% <0.00%> (-100.00%) ⬇️
...pinot/minion/exception/TaskCancelledException.java 0.00% <0.00%> (-100.00%) ⬇️
... and 427 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Jackie-Jiang
Copy link
Contributor

The quick-start is failing because of this change (see the comment in the changed dependency file for details). Do you think you can override the dependency in your build instead of changing the dependency scope here? That is usually possible if you pin the library version on the top level

@jadami10
Copy link
Contributor Author

Good point. We actually made this change and commented that command out in our own build. I can't think of a better way that doesn't require users to provide the SparkLaucher in the class path of the admin starter, so we'll just leave this changed.

@jadami10 jadami10 closed this Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants