You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "abhioncbr (via GitHub)" <gi...@apache.org> on 2023/03/20 00:47:24 UTC
[GitHub] [pinot] abhioncbr opened a new pull request, #10446: 10218: Added log statement for jvm options.
abhioncbr opened a new pull request, #10446:
URL: https://github.com/apache/pinot/pull/10446
- Added log statement for JVM options.
--
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] abhioncbr commented on pull request #10446: 10218: Added log statement for jvm options.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10446:
URL: https://github.com/apache/pinot/pull/10446#issuecomment-1479884085
> > @swaminathanmanish can you please review the changes?
>
> Thanks for taking this up. Actually we already have a nice swagger API that dumps all configs which is convenient to use. We don't need to look at the logs :). Do we really need these logs?
>
> curl -X GET "http://localhost:9000/appconfigs" -H "accept: application/json"
>
> jvmConfig": { "args": [ "-javaagent:/Applications/IntelliJ IDEA CE.app/Contents/lib/idea_rt.jar=50367:/Applications/IntelliJ IDEA CE.app/Contents/bin", "-Dfile.encoding=UTF-8" ], "garbageCollectors": [ "G1 Young Generation", "G1 Old Generation" ], "envVariables": { "__CFBundleIdentifier": "com.jetbrains.intellij.ce", "PATH": "/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin", "SHELL": "/bin/zsh", "MANPATH": "/opt/homebrew/share/man::", "HOMEBREW_CELLAR": "/opt/homebrew/Cellar", "OLDPWD": "/", "USER": "manishswaminathan", "HOMEBREW_PREFIX": "/opt/homebrew", "COMMAND_MODE": "unix2003", "TMPDIR": "/var/folders/xj/yl13brv573g9_v_vhxx7knx00000gn/T/", "SSH_AUTH_SOCK": "/private/tmp/com.apple.launchd.lyuZIHVl7g/Listeners", "XPC_FLAGS": "0x0", "__CF_USER_TEXT_ENCODING": "0x1F5:0x0:0x0", "LOGNAME": "manishswaminathan", "LC_CTYPE": "en_US.UTF-8", "HOMEBREW_REPOSITORY": "/opt/homebrew", "JAVA_MAIN_CLASS_1666": "org.apache.pinot
.tools.Quickstart", "PWD": "/Users/manishswaminathan/projects/forked/pinot", "XPC_SERVICE_NAME": "application.com.jetbrains.intellij.ce.975373.976077", "INFOPATH": "/opt/homebrew/share/info:", "HOME": "/Users/manishswaminathan" },
I think, there is no harm in adding up the logs if it makes someone's life easy. Rest, I have no such strong preferences to add to logs.
--
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 merged pull request #10446: 10218: Added log statement for jvm options.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10446:
URL: https://github.com/apache/pinot/pull/10446
--
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] abhioncbr commented on pull request #10446: 10218: Added log statement for jvm options.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10446:
URL: https://github.com/apache/pinot/pull/10446#issuecomment-1479669616
@swaminathanmanish can you please review the changes?
--
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] swaminathanmanish commented on pull request #10446: 10218: Added log statement for jvm options.
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on PR #10446:
URL: https://github.com/apache/pinot/pull/10446#issuecomment-1479903271
> jvm options ( such as Xmx for example ) used by pinot processes should be logged #10218
Yes logging will do no harm but swagger apis are more convenient, centralized, should be source of truth as well. Perhaps t[he wants these logs to look at historical configs ](https://github.com/apache/pinot/issues/10218)as well which is not available via the API? In that case logging makes sense.
--
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 #10446: 10218: Added log statement for jvm options.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10446:
URL: https://github.com/apache/pinot/pull/10446#issuecomment-1475487585
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10446?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10446](https://codecov.io/gh/apache/pinot/pull/10446?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0f5506c) into [master](https://codecov.io/gh/apache/pinot/commit/d9c4315ca14997e6e0300d04f788e07723875159?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d9c4315) will **decrease** coverage by `50.30%`.
> The diff coverage is `28.57%`.
```diff
@@ Coverage Diff @@
## master #10446 +/- ##
=============================================
- Coverage 64.21% 13.92% -50.30%
+ Complexity 6089 237 -5852
=============================================
Files 2007 2008 +1
Lines 109281 109288 +7
Branches 16692 16692
=============================================
- Hits 70177 15215 -54962
- Misses 33993 92837 +58844
+ Partials 5111 1236 -3875
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `?` | |
| unittests2 | `13.92% <28.57%> (+<0.01%)` | :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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10446?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...n/java/org/apache/pinot/common/utils/JvmUtils.java](https://codecov.io/gh/apache/pinot/pull/10446?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvSnZtVXRpbHMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ava/org/apache/pinot/minion/BaseMinionStarter.java](https://codecov.io/gh/apache/pinot/pull/10446?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vQmFzZU1pbmlvblN0YXJ0ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/pinot/pull/10446?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `72.72% <100.00%> (+0.11%)` | :arrow_up: |
| [...apache/pinot/controller/BaseControllerStarter.java](https://codecov.io/gh/apache/pinot/pull/10446?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9CYXNlQ29udHJvbGxlclN0YXJ0ZXIuamF2YQ==) | `79.04% <100.00%> (+0.06%)` | :arrow_up: |
... and [1356 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10446/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
: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=The+Apache+Software+Foundation)
--
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] abhioncbr commented on pull request #10446: 10218: Added log statement for jvm options.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10446:
URL: https://github.com/apache/pinot/pull/10446#issuecomment-1476514990
> @abhioncbr Could this be included in PinotAppConfigs instead, here? https://github.com/apache/pinot/blob/master/pinot-common/src/main/java/org/apache/pinot/common/utils/PinotAppConfigs.java#L64
Sure. Thanks for the review.
--
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 a diff in pull request #10446: 10218: Added log statement for jvm options.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10446:
URL: https://github.com/apache/pinot/pull/10446#discussion_r1149798237
##########
pinot-tools/src/main/java/org/apache/pinot/tools/service/PinotServiceManager.java:
##########
@@ -107,6 +108,7 @@ public String startRole(ServiceRole role, Map<String, Object> properties)
public String startController(String controllerStarterClassName, PinotConfiguration controllerConf)
throws Exception {
LOGGER.info("Trying to start Pinot Controller...");
+ LOGGER.info(new PinotAppConfigs(controllerConf).getJvmInputArguments());
Review Comment:
I don't think we need to log here because it will be logged within the component starter
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -223,6 +224,7 @@ public PinotConfiguration getConfig() {
public void start()
throws Exception {
LOGGER.info("Starting Pinot broker (Version: {})", PinotVersion.VERSION);
+ LOGGER.info(new PinotAppConfigs(getConfig()).getJvmInputArguments());
Review Comment:
We can probably log everything within the `PinotAppConfigs` by calling:
```suggestion
LOGGER.info("Broker configs: {}", new PinotAppConfigs(getConfig()).toJSONString());
```
Same for other components
--
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] abhioncbr commented on a diff in pull request #10446: 10218: Added log statement for jvm options.
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10446:
URL: https://github.com/apache/pinot/pull/10446#discussion_r1149890188
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -223,6 +224,7 @@ public PinotConfiguration getConfig() {
public void start()
throws Exception {
LOGGER.info("Starting Pinot broker (Version: {})", PinotVersion.VERSION);
+ LOGGER.info(new PinotAppConfigs(getConfig()).getJvmInputArguments());
Review Comment:
Thanks for the review. Let me make the changes.
--
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] swaminathanmanish commented on pull request #10446: 10218: Added log statement for jvm options.
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on PR #10446:
URL: https://github.com/apache/pinot/pull/10446#issuecomment-1479871501
> @swaminathanmanish can you please review the changes?
Thanks for taking this up. Actually we already have a nice swagger API that dumps all configs which is convenient to use. We don't need to look at the logs :). Do we really need these logs?
curl -X GET "http://localhost:9000/appconfigs" -H "accept: application/json"
jvmConfig": {
"args": [
"-javaagent:/Applications/IntelliJ IDEA CE.app/Contents/lib/idea_rt.jar=50367:/Applications/IntelliJ IDEA CE.app/Contents/bin",
"-Dfile.encoding=UTF-8"
],
"garbageCollectors": [
"G1 Young Generation",
"G1 Old Generation"
],
"envVariables": {
"__CFBundleIdentifier": "com.jetbrains.intellij.ce",
"PATH": "/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin",
"SHELL": "/bin/zsh",
"MANPATH": "/opt/homebrew/share/man::",
"HOMEBREW_CELLAR": "/opt/homebrew/Cellar",
"OLDPWD": "/",
"USER": "manishswaminathan",
"HOMEBREW_PREFIX": "/opt/homebrew",
"COMMAND_MODE": "unix2003",
"TMPDIR": "/var/folders/xj/yl13brv573g9_v_vhxx7knx00000gn/T/",
"SSH_AUTH_SOCK": "/private/tmp/com.apple.launchd.lyuZIHVl7g/Listeners",
"XPC_FLAGS": "0x0",
"__CF_USER_TEXT_ENCODING": "0x1F5:0x0:0x0",
"LOGNAME": "manishswaminathan",
"LC_CTYPE": "en_US.UTF-8",
"HOMEBREW_REPOSITORY": "/opt/homebrew",
"JAVA_MAIN_CLASS_1666": "org.apache.pinot.tools.Quickstart",
"PWD": "/Users/manishswaminathan/projects/forked/pinot",
"XPC_SERVICE_NAME": "application.com.jetbrains.intellij.ce.975373.976077",
"INFOPATH": "/opt/homebrew/share/info:",
"HOME": "/Users/manishswaminathan"
},
--
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] swaminathanmanish commented on pull request #10446: 10218: Added log statement for jvm options.
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on PR #10446:
URL: https://github.com/apache/pinot/pull/10446#issuecomment-1476464323
@abhioncbr Could this be included in PinotAppConfigs instead, here?
https://github.com/apache/pinot/blob/master/pinot-common/src/main/java/org/apache/pinot/common/utils/PinotAppConfigs.java#L64
--
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