Added config toggle for verbose GC logging #3663

nicknezis · 2021-01-12T19:26:46Z

I added a config item to toggle the verbose GC logging. Not sure if this is the best way to do it. There was another option to add it to the heron_internals.yml. In the end I thought this might be the better approach because it would allow for the analytic to set the value.

I also removed some of the pre-JDK9 config options because Heron is now on JDK 11.

heron/executor/src/python/heron_executor.py

nicknezis · 2021-01-12T23:09:08Z

This fixes #3659

Co-authored-by: Oliver Bristow <evilumbrella+github@gmail.com>

Code0x58

At least the python side appears ok. I can't approve on the Java side without further reading, but I'd double check the naming and how it connects to the executor (particularly as this may be the first is_flag example so my earlier suggestion may case issues, and this may be the first Boolean option)

nicknezis · 2021-01-13T05:08:20Z

I'm going to do some actual testing before merging, so hopefully will catch any name mismatches.

nwangtw

Overall LGTM other than the question about "-Xloggc".

nwangtw · 2021-01-14T15:53:19Z

heron/executor/tests/python/heron_executor_unittest.py

+           "-Djava.net.preferIPv4Stack=true " \
+           "-XX:+UseG1GC -XX:+ParallelRefProcEnabled -XX:+UseStringDeduplication " \
+           "-XX:MaxGCPauseMillis=100 -XX:InitiatingHeapOccupancyPercent=30 " \
+           "-XX:+HeapDumpOnOutOfMemoryError -XX:ParallelGCThreads=4 " \


"Xloggc:log-files/gc.%s.log" and the instance_name are still necessary I feel since all the logs are in the same folder.

-Xloggc set of flags are all related to the GC logging. In my testing, I still saw JVM component log files in the log-files folder. This change should only remove the GC specific logging (unless someone chooses to enable at submit time).

I believe if the verbose-gc flag is not set, the changes I made to the code will not set those JVM flags. This unit test is testing with the defaults, so what you see here matches what was generated by the heron_executor.py.

Yeah. The flag is for gc logs only, but it separates different gc logs into different files. For example, the gc log for this process would be "log-files/gc.healthmgr.log" and each process has its own gc log file.

I think the heron_executor functionality that was setting that path and log file names is still there. It's just not there for the default submission. You have to use the flag to enable those parameters.

These lines of code that you are commenting on are test results for the default case that does not have verbose_gc enabled. If there was a test for the verbose_gc use case, then we would have another version of this string that included the various GC log parameters. I'm not sure how much more work it will be to add another test case that captures that scenario, but I will look into making it.

nwangtw · 2021-01-14T15:54:15Z

heron/executor/tests/python/heron_executor_unittest.py

-           "-XX:GCLogFileSize=100M -XX:+PrintPromotionFailure -XX:+PrintTenuringDistribution " \
-           "-XX:+PrintHeapAtGC -XX:+HeapDumpOnOutOfMemoryError -XX:ParallelGCThreads=4 " \
-           "-Xloggc:log-files/gc.healthmgr.log " \
+           "-Djava.net.preferIPv4Stack=true " \


"-Xloggc:log-files" might be necessary.

Same as comment above. I believe -Xloggc:log-files is a GC specific setting. This does not impact the default logging.

nwangtw · 2021-01-14T15:54:28Z

heron/executor/tests/python/heron_executor_unittest.py

+           "-Djava.net.preferIPv4Stack=true " \
+           "-XX:+UseG1GC -XX:+ParallelRefProcEnabled -XX:+UseStringDeduplication " \
+           "-XX:MaxGCPauseMillis=100 -XX:InitiatingHeapOccupancyPercent=30 " \
+           "-XX:+HeapDumpOnOutOfMemoryError -XX:ParallelGCThreads=4 " \


"-Xloggc:log-files" might be necessary.

nwangtw · 2021-01-14T15:54:32Z

heron/executor/tests/python/heron_executor_unittest.py

+           "-Djava.net.preferIPv4Stack=true " \
+           "-XX:+UseG1GC -XX:+ParallelRefProcEnabled -XX:+UseStringDeduplication " \
+           "-XX:MaxGCPauseMillis=100 -XX:InitiatingHeapOccupancyPercent=30 " \
+           "-XX:+HeapDumpOnOutOfMemoryError -XX:ParallelGCThreads=4 " \


"-Xloggc:log-files" might be necessary.

nicknezis · 2021-01-14T16:23:08Z

@nwangtw Is it ok that I modified the Heron CLI to add the new argument? In the future I want to also add a flag to toggle the heap dump JVM feature. Would I also add that as a flag on the CLI? It's there a more generic way to provide overrides per analytic submission when submitting to a remote Heron server?

nwangtw · 2021-01-14T16:29:55Z

@nwangtw Is it ok that I modified the Heron CLI to add the new argument? In the future I want to also add a flag to toggle the heap dump JVM feature. Would I also add that as a flag on the CLI? It's there a more generic way to provide overrides per analytic submission when submitting to a remote Heron server?

Yeah. I think passing from CLI could be very useful. I remember we can pass extra topology configs from CLI now but I can't remember the details. @huijunw @xiaoyao1991 ?

joshfischer1108 · 2021-01-14T16:49:11Z

@nwangtw Is it ok that I modified the Heron CLI to add the new argument? In the future I want to also add a flag to toggle the heap dump JVM feature. Would I also add that as a flag on the CLI? It's there a more generic way to provide overrides per analytic submission when submitting to a remote Heron server?

Yeah. I think passing from CLI could be very useful. I remember we can pass extra topology configs from CLI now but I can't remember the details. @huijunw @xiaoyao1991 ?

I think it's here:

Something like --config-property PROPERTY=VALUE

incubator-heron/heron/tools/cli/src/python/args.py

Line 120 in 0f0ba18

parser.add_argument(

nicknezis · 2021-01-20T21:58:55Z

@Code0x58 Github is still showing that you have "1 change requested". I'm not sure if that is something I can resolve or if you need to re-review to clear that state.

nicknezis · 2021-01-20T21:59:19Z

Also a general re-review is probably warranted. I was able to test running an analytic with and without the flag and it seemed to operate as I wanted.

Added config toggle for verbose GC logging

9624546

nicknezis requested review from nwangtw and Code0x58 January 12, 2021 19:26

nicknezis self-assigned this Jan 12, 2021

nwangtw reviewed Jan 12, 2021

View reviewed changes

heron/executor/src/python/heron_executor.py Outdated Show resolved Hide resolved

Fixed typo

3de59e3

Code0x58 suggested changes Jan 12, 2021

View reviewed changes

heron/executor/src/python/heron_executor.py Outdated Show resolved Hide resolved

heron/executor/src/python/heron_executor.py Outdated Show resolved Hide resolved

heron/executor/src/python/heron_executor.py Outdated Show resolved Hide resolved

Code0x58 reviewed Jan 12, 2021

View reviewed changes

heron/executor/src/python/heron_executor.py Outdated Show resolved Hide resolved

nicknezis and others added 4 commits January 12, 2021 18:10

Update heron/executor/src/python/heron_executor.py

1c29e0a

Co-authored-by: Oliver Bristow <evilumbrella+github@gmail.com>

Update heron/executor/src/python/heron_executor.py

e4fe1dd

Co-authored-by: Oliver Bristow <evilumbrella+github@gmail.com>

Update heron/executor/src/python/heron_executor.py

b6da4b6

Co-authored-by: Oliver Bristow <evilumbrella+github@gmail.com>

Update heron/executor/src/python/heron_executor.py

a2954e1

Co-authored-by: Oliver Bristow <evilumbrella+github@gmail.com>

nicknezis marked this pull request as ready for review January 12, 2021 23:11

nicknezis requested a review from Code0x58 January 12, 2021 23:11

Code0x58 reviewed Jan 13, 2021

View reviewed changes

Code0x58 self-requested a review January 13, 2021 03:11

nicknezis added 9 commits January 13, 2021 02:55

Updated the logic so that it can be configured at submission time

9a1fff6

Style fix

a3eabf4

Style fix

f7bca95

Updated heron_executor_unittest

a57c798

Removing unnecessary comments

d47c857

Added verbose-gc flag to the Heron CLI

eaa23ec

Adding missing argument

a95896c

Forgot to make the argument a boolean

afc3721

Correcting type

f8797a0

nwangtw approved these changes Jan 14, 2021

View reviewed changes

More updates

de0e609

nicknezis requested review from huijunwu and windhamwong January 20, 2021 07:27

Code0x58 approved these changes Jan 20, 2021

View reviewed changes

Style cleanup

4f73981

nicknezis merged commit f11ebc3 into apache:master Jan 21, 2021

nicknezis deleted the nicknezis/verbose-gc-toggle branch January 21, 2021 05:33

nicknezis mentioned this pull request Jan 22, 2021

Add config toggle for turning off Verbose JVM GC output #3659

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added config toggle for verbose GC logging #3663

Added config toggle for verbose GC logging #3663

nicknezis commented Jan 12, 2021 •

edited

Loading

nicknezis commented Jan 12, 2021

Code0x58 left a comment

nicknezis commented Jan 13, 2021

nwangtw left a comment

nwangtw Jan 14, 2021

nicknezis Jan 14, 2021

nwangtw Jan 14, 2021

nicknezis Jan 14, 2021

nwangtw Jan 14, 2021

nicknezis Jan 14, 2021

nwangtw Jan 14, 2021

nwangtw Jan 14, 2021

nicknezis commented Jan 14, 2021

nwangtw commented Jan 14, 2021

joshfischer1108 commented Jan 14, 2021

nicknezis commented Jan 20, 2021

nicknezis commented Jan 20, 2021

Added config toggle for verbose GC logging #3663

Added config toggle for verbose GC logging #3663

Conversation

nicknezis commented Jan 12, 2021 • edited Loading

nicknezis commented Jan 12, 2021

Code0x58 left a comment

Choose a reason for hiding this comment

nicknezis commented Jan 13, 2021

nwangtw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicknezis commented Jan 14, 2021

nwangtw commented Jan 14, 2021

joshfischer1108 commented Jan 14, 2021

nicknezis commented Jan 20, 2021

nicknezis commented Jan 20, 2021

nicknezis commented Jan 12, 2021 •

edited

Loading