Skip to content

Make Pinot Broker/Server/Minion can start by just passing a config file #5446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
xiangfu0 merged 2 commits into from
May 24, 2021
Merged

Make Pinot Broker/Server/Minion can start by just passing a config file #5446

xiangfu0 merged 2 commits into from
May 24, 2021

Conversation

@xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented May 26, 2020 •
edited
Loading

  • Adding constants for helix cluster name and zookeeper server
  • Create construct for HelixBrokerStarter and HelixServerStarter to use just Configuration and move all Pinot internal usage to use a new constructor.
  • Deprecated old constructors for HelixBrokerStarter and HelixServerStarter
  • Update Pinot Controller to also use helix cluster and zk server constant but remain backward compatibility.
  • Adding sample default pinot-<controller/broker/server> config file.

After this change, users can run the below commands to launch airlineStats example:

bin/pinot-admin.sh StartController -configFileName conf/pinot-controller.conf

bin/pinot-admin.sh StartBroker -configFileName conf/pinot-broker.conf

bin/pinot-admin.sh StartServer -configFileName conf/pinot-server.conf

bin/pinot-admin.sh StartMinion -configFileName conf/pinot-minion.conf

bin/pinot-admin.sh AddTable  -schemaFile examples/batch/airlineStats/airlineStats_schema.json -tableConfigFile examples/batch/airlineStats/airlineStats_offline_table_config.json -exec

bin/pinot-admin.sh LaunchDataIngestionJob  -jobSpecFile examples/batch/airlineStats/ingestionJobSpec.yaml

@xiangfu0 xiangfu0 force-pushed the default_pinot_startable_to_use_config_files branch 6 times, most recently from 17fc399 to 51b3e96 Compare May 26, 2020 09:51
@kishoreg
Copy link
Member

kishoreg commented May 26, 2020

Let's ensure all parameter names follow the same convention - config or configFile or configFileName

all config seems to have prefix as well to indicate what config it is

so maybe something like this

  • serverConfigFile
  • brokerConfigFile
  • tableConfigFile
  • schemFile
  • jobSpecFile

@xiangfu0
Copy link
Contributor Author

xiangfu0 commented May 26, 2020

Let's ensure all parameter names follow the same convention - config or configFile or configFileName

all config seems to have prefix as well to indicate what config it is

so maybe something like this

  • serverConfigFile
  • brokerConfigFile
  • tableConfigFile
  • schemFile
  • jobSpecFile

updated by adding configs alias.

@xiangfu0 xiangfu0 force-pushed the default_pinot_startable_to_use_config_files branch from 8626188 to 84ff2ff Compare May 28, 2020 23:03
@xiangfu0 xiangfu0 force-pushed the default_pinot_startable_to_use_config_files branch from 84ff2ff to 538955c Compare June 12, 2020 21:41
@xiangfu0 xiangfu0 force-pushed the default_pinot_startable_to_use_config_files branch 6 times, most recently from 36e584e to 056a3b0 Compare May 20, 2021 00:38
@xiangfu0 xiangfu0 changed the title Make Pinot Broker/Server can start by just passing a config file Make Pinot Broker/Server/Minion can start by just passing a config file May 20, 2021
@xiangfu0 xiangfu0 force-pushed the default_pinot_startable_to_use_config_files branch from 056a3b0 to c431653 Compare May 20, 2021 00:47
mayankshriv
mayankshriv approved these changes May 20, 2021
View reviewed changes
Copy link
Contributor

@mayankshriv mayankshriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address open comments.

pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/HelixBrokerStarter.java Outdated
Copy link
Contributor

@mayankshriv mayankshriv May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new method that is already deprecated :-)?

Copy link
Contributor Author

@xiangfu0 xiangfu0 May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only way to use the new constructor :p

pinot-tools/src/main/resources/conf/pinot-broker.conf Outdated
Copy link
Contributor

@mayankshriv mayankshriv May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a single conf file for the entire cluster, so that user doesn't need to maintain three/four separate conf files?

Copy link
Contributor Author

@xiangfu0 xiangfu0 May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense, I will add one for StartServiceManager

Copy link
Contributor Author

@xiangfu0 xiangfu0 May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have a single conf file right now as the StartServiceManager requires a role to start pinot, which is the same key across all the configs.

Current way to start all-in-one container is to give 3 conf files, e.g.

bin/pinot-admin.sh StartServiceManager -bootstrapConfigPaths conf/pinot-controller.conf conf/pinot-broker.conf conf/pinot-server.conf

@xiangfu0 xiangfu0 force-pushed the default_pinot_startable_to_use_config_files branch 2 times, most recently from fe5527c to 548eba4 Compare May 20, 2021 18:53
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2021 •
edited
Loading

Codecov Report

Merging #5446 (b867987) into master (fe322f5) will decrease coverage by 0.09%.
The diff coverage is 43.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5446      +/-   ##
============================================
- Coverage     73.36%   73.27%   -0.10%     
  Complexity       12       12              
============================================
  Files          1432     1432              
  Lines         70854    71034     +180     
  Branches      10258    10291      +33     
============================================
+ Hits          51984    52049      +65     
- Misses        15414    15498      +84     
- Partials       3456     3487      +31
Flag Coverage Δ Complexity Δ
integration 42.99% <35.21%> (-0.02%) 7.00 <0.00> (ø)
unittests 65.32% <30.98%> (-0.08%) 12.00 <0.00> (ø)

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

Impacted Files Coverage Δ Complexity Δ
...a/org/apache/pinot/spi/env/PinotConfiguration.java 97.53% <0.00%> (-2.47%) 0.00 <0.00> (ø)
...va/org/apache/pinot/spi/utils/CommonConstants.java 29.82% <ø> (ø) 0.00 <0.00> (ø)
...va/org/apache/pinot/controller/ControllerConf.java 53.66% <21.42%> (-2.10%) 0.00 <0.00> (ø)
.../pinot/broker/broker/helix/HelixBrokerStarter.java 63.87% <23.52%> (-4.46%) 0.00 <0.00> (ø)
...pinot/server/starter/helix/HelixServerStarter.java 49.03% <27.27%> (-0.97%) 0.00 <0.00> (ø)
...in/java/org/apache/pinot/minion/MinionStarter.java 76.31% <37.50%> (-3.13%) 0.00 <0.00> (ø)
...g/apache/pinot/spi/utils/PinotReflectionUtils.java 75.00% <85.71%> (-5.00%) 0.00 <0.00> (ø)
...pinot/broker/broker/BrokerAdminApiApplication.java 92.50% <100.00%> (+0.39%) 0.00 <0.00> (ø)
.../controller/api/ControllerAdminApiApplication.java 91.83% <100.00%> (+0.34%) 0.00 <0.00> (ø)
...apache/pinot/minion/MinionAdminApiApplication.java 91.66% <100.00%> (+0.49%) 0.00 <0.00> (ø)
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe322f5...b867987. Read the comment docs.

@xiangfu0 xiangfu0 force-pushed the default_pinot_startable_to_use_config_files branch 3 times, most recently from 66510fe to 6a6ddfa Compare May 23, 2021 10:59
@xiangfu0 xiangfu0 force-pushed the default_pinot_startable_to_use_config_files branch 8 times, most recently from bc277be to a212a9a Compare May 24, 2021 06:06
@xiangfu0 xiangfu0 force-pushed the default_pinot_startable_to_use_config_files branch from a212a9a to b867987 Compare May 24, 2021 06:14
@xiangfu0 xiangfu0 merged commit 71e156c into master May 24, 2021
@xiangfu0 xiangfu0 deleted the default_pinot_startable_to_use_config_files branch May 24, 2021 07:40
@Jackie-Jiang Jackie-Jiang mentioned this pull request Aug 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mayankshriv mayankshriv mayankshriv approved these changes

@kishoreg kishoreg Awaiting requested review from kishoreg

@Jackie-Jiang Jackie-Jiang Awaiting requested review from Jackie-Jiang

@npawar npawar Awaiting requested review from npawar

@mcvsubbu mcvsubbu Awaiting requested review from mcvsubbu

@siddharthteotia siddharthteotia Awaiting requested review from siddharthteotia

@jackjlli jackjlli Awaiting requested review from jackjlli

@snleee snleee Awaiting requested review from snleee

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xiangfu0 @kishoreg @codecov-commenter @mayankshriv