KAFKA-7834: Extend collected logs in system test services to include heap dumps by kkonstantine · Pull Request #6158 · apache/kafka

kkonstantine · 2019-01-16T19:40:12Z

Enable heap dumps on OOM with -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=<file.bin> in the major services in system tests
Collect the heap dump from the predefined location as part of the result logs for each service
Change Connect service to delete the whole root directory instead of individual expected files
Tested by running the full suite of system tests

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ewencp · 2019-01-22T03:28:41Z

are these all under PERSISTENT_ROOT? if yes, can we just handle PERSISTENT_ROOT and not worry about individual files?

the other_files are not necessarily under PERSISTENT_ROOT
What is under it for sure is:
self.CONFIG_FILE, self.LOG4J_CONFIG_FILE, self.PID_FILE, self.LOG_FILE, self.STDOUT_FILE, self.STDERR_FILE, self.EXTERNAL_CONFIGS_FILE which I removed from explicit listing and I'm removing with PERSISTENT_ROOT altogether.

ok, I get that though I think that's just tech debt. for any test related files, we really shouldn't be using anything other than PERSISTENT_ROOT so that we can at least attempt to ensure each test/service gets a clean workspace

ewencp · 2019-01-22T03:32:44Z

we seem to duplicate this pattern of setting constants, is there any opportunity to relocate and reduce to one copy of these constants (given they should be same across all our java services)?

With each service setting its security configs in KAFKA_OPTS using their own way, this seems difficult at the moment. I'd prefer if I could avoid to extend this PR to a general refactoring at this point.

ewencp · 2019-01-22T03:35:21Z

what is the logic for this, including the different ways env_opts is being encoded?

I ended up adding the heap jvm arguments in KAFKA_OPTS instead of KAFKA_HEAP_OPTS because the latter has defaults for sizes if it's empty. However, if I needed to add values to an env variable, I can't just append them to it's current value, because the value is wrapped with \"

That's why I'm adding this method - unused here - that can be used by tests that inherit this class. Let me know if you think I should remove.

yeah, in general the construction of commands could be cleaned up quite a bit for extensibility -- i think we tend to forget about the importance/value of doing this when writing tests in the Kafka repo, but these classes should be reusable and extensible. this approach seems pretty error-prone, although at least it should consistently fail if the quoting gets messed up since you'll have invalid extra args on the command line.

ewencp · 2019-01-22T03:36:18Z

what's happening with the various strip('\"') calls? why do we need this process? can we refactor to something that makes it clearer what's happening?

As I mention above, we often wrap the value of an env var in escaped double quotes. To append contents, we first need to remove those. If they don't exist, strip is a no-op.

ewencp · 2019-01-22T03:37:45Z

this seems unrelated to this PR? and the bucket probably shouldn't change unless we're changing the target bucket -- we want only 1, permanent bucket if possible

It is temporary. Was added with the commit named: TEMP commit to unblock system test branch builder in order to fix an issue with the jenkins builder and be able to run the tests. Should be removed before merging

ewencp

LGTM. The JIRA is marked to be backported to 1.0, but it looks like this only backports cleanly to 2.0. Additionally, 2.1.1 is currently in flight, and although this is only test code and I don't think too risky, I don't want to merge until we get the next RC out the door. So I may hold off on the actual merge here until that passes.

ewencp · 2019-02-04T23:16:28Z

ok, I get that though I think that's just tech debt. for any test related files, we really shouldn't be using anything other than PERSISTENT_ROOT so that we can at least attempt to ensure each test/service gets a clean workspace

ewencp · 2019-02-05T00:19:03Z

yeah, in general the construction of commands could be cleaned up quite a bit for extensibility -- i think we tend to forget about the importance/value of doing this when writing tests in the Kafka repo, but these classes should be reusable and extensible. this approach seems pretty error-prone, although at least it should consistently fail if the quoting gets messed up since you'll have invalid extra args on the command line.

ewencp · 2019-02-05T00:38:49Z

Actually, after further discussion, will merge to trunk, 2.2, and 2.0 so we can get debugging information out for some failing tests. Will follow up on 2.1 after it won't get in the way of 2.1.1 release, and we'll follow up separately with 1.x versions if we find the additional info to be valuable, but hopefully tests shouldn't be that unstable on long term maintenance branches.

kkonstantine · 2019-02-05T00:41:33Z

Excellent. Thanks @ewencp! I'll keep track progress with respect to 2.1.1

…heap dumps * Enable heap dumps on OOM with -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=<file.bin> in the major services in system tests * Collect the heap dump from the predefined location as part of the result logs for each service * Change Connect service to delete the whole root directory instead of individual expected files * Tested by running the full suite of system tests Author: Konstantine Karantasis <konstantine@confluent.io> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io> Closes #6158 from kkonstantine/KAFKA-7834 (cherry picked from commit 83c435f) Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>

ewencp · 2019-02-20T02:17:18Z

Now that 2.1.1 has finally landed, cherry-picking this into 2.1 as well. Seems like it has been fine for awhile on other branches anyway so probably we could have risked it even earlier.

…heap dumps * Enable heap dumps on OOM with -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=<file.bin> in the major services in system tests * Collect the heap dump from the predefined location as part of the result logs for each service * Change Connect service to delete the whole root directory instead of individual expected files * Tested by running the full suite of system tests Author: Konstantine Karantasis <konstantine@confluent.io> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io> Closes #6158 from kkonstantine/KAFKA-7834

…heap dumps * Enable heap dumps on OOM with -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=<file.bin> in the major services in system tests * Collect the heap dump from the predefined location as part of the result logs for each service * Change Connect service to delete the whole root directory instead of individual expected files * Tested by running the full suite of system tests Author: Konstantine Karantasis <konstantine@confluent.io> Reviewers: Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#6158 from kkonstantine/KAFKA-7834

ewencp reviewed Jan 22, 2019

View reviewed changes

kkonstantine force-pushed the KAFKA-7834 branch 7 times, most recently from 38d4110 to 7775110 Compare January 24, 2019 01:48

kkonstantine added 4 commits January 25, 2019 14:26

KAFKA-7834: Enable and collect heap dump for ZK system test service

68af2bc

KAFKA-7834: Enable and collect heap dump for Kafka system test service

3286d69

KAFKA-7834: Enable and collect heap dump for Connect system test service

c30cf90

KAFKA-7834: Change Connect service to delete its root directory

b964036

kkonstantine force-pushed the KAFKA-7834 branch from 7775110 to b964036 Compare January 25, 2019 22:26

kkonstantine mentioned this pull request Jan 28, 2019

MINOR: Upgrade ducktape to 0.7.5 #6197

Merged

3 tasks

ewencp approved these changes Feb 5, 2019

View reviewed changes

ewencp closed this in 83c435f Feb 5, 2019

kkonstantine deleted the KAFKA-7834 branch February 5, 2019 05:21

kkonstantine added connect tests Test fixes (including flaky tests) labels Oct 16, 2020

Conversation

kkonstantine commented Jan 16, 2019

Committer Checklist (excluded from commit message)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewencp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewencp commented Feb 5, 2019

Uh oh!

kkonstantine commented Feb 5, 2019

Uh oh!

ewencp commented Feb 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants