KAFKA-2669; Fix LogCleanerIntegrationTest by lindong28 · Pull Request #327 · apache/kafka

lindong28 · 2015-10-17T09:18:16Z

LogCleanerIntegrationTest calls LogCleaner.awaitCleaned() to wait until cleaner has processed up to given offset. However, existing awaitCleaned() implementation doesn't wait for this. This patch fix the problem.

lindong28 · 2015-10-17T09:22:41Z

In addition, MockScheduler should execute all remaining tasks when it is shutdown. This is useful, e.g., to execute scheduled file deletion task in LogCleanerIntegrationTest so that we avoid "too many open file" exception.

ijuma · 2015-10-17T09:32:24Z

The following is more idiomatic:

while (cleanerManager.allCleanerCheckpoints.get(TopicAndPartition(topic, part)).fold(true)(_ < offset) Thread.sleep(10)

Nice approach. I will follow your suggestion.

lindong28 · 2015-10-17T17:49:20Z

@auradkar @ijuma Thanks for your suggestion. I have updated patch accordingly.

auradkar · 2015-10-17T19:21:18Z

Can you check if this is only used in a test? Ideally, we should use the timeout to prevent indefinite blocking

Yeah, we don't want the test to hang forever instead of failing with a time out.

Sure. I have verified that this is only used in test.

Also note that the "timeout" is not specified in API, not used in any invocation, or implemented in the function. If I were to implement this, I need to find a good default value that doesn't break existing test. Since it is only used in the test, I prefer to leave it as it is and only implement it when we have a good usecase.

@lindong28 The bug here is that the timeout was not implemented in the function. Because there is a default value (30000L), callers are expecting that timeout so I don't agree with your assessment that it is not used in invocations. This pattern is really common in our tests (see TestUtils.waitUntil* methods) and callers usually don't override the default timeout. Given that, I think we already have a use-case, it is why the parameter exists in the first place.

@ijuma Yeah I know we have default value of 30000L for timeout. My point is that, since it is not explicitly specified in the invocation it probably means users don't care about it. It is important that, if we enable this timeout, existing tests won't fail due to low timeout value.

I am not sure.. but can you explain why 30000 ms is a good default value for timeout? Note that log cleaner's default backoffMs is 15000 ms.

Alternatively, how about I set timeout to be LONG.MAX by default? This won't accidentally fail existing test while still allow developers to use timeout when they want.

I disagree with the assertion that because the callers didn't explicitly set the timeout, they don't care about a timeout. We have several examples of callers of TestUtils.waitUntilTrue that care about a timeout, but don't set it explicitly because the default is OK (that's a big reason why default arguments are useful).

I don't know what is a good default for this method, but I certainly know that we can't wait indefinitely for things in tests. It means that a bug can cause the whole test suite to hang, which is a very bad outcome (even worse than accidentally failing tests, which is also bad).

Ideally we'd set a timeout that would not cause tests to fail, but that wouldn't delay the test suite by too long if it were to fail. It's OK to err on the side of caution, but Long.MaxValue is far too long. Why not choose a value that you think is appropriate and then run the test suite a few times in a loop to see if it's OK?

I have already run LogCleanerIntegrationTest and DeleteTopicTest a few times with infinite timeout and the tests run well. That is the reason why I think inifinite timeout works for existing tests. I don't think setting a default timeout value that is large enough for existing invocation in tests is a very good idea -- a default time value should ideally be good for most usecase from first principle.

But I agree with your point that we should never allow tests to run forever. How about I make the following change:

give default timeout value of 60*1000 ms. This is 4X the default backoffMs which is long enough for existing tests.

let awaitCleaned return a boolean value to indicate whether it has timed out or not -- this allows the tests to fail in case of timeout so that we can look into the problem.

Chiming late here: I also feel setting an infinite timeout value is generally not a good idea since IF there are any issue causing the test to block, it is hard to detect this issue: for example today we also encounter blocking-forever test cases in Jenkins from time to time, and because we set the Jenkins time to 1 hour (? not sure if the value is exact), we ended up seeing an "timed out" failed Jenkins without much information which test case caused it.

I think the current approach of setting a large enough default value is a better option.

Thanks for the comment. I definitely agree that we should prevent test from running forever.

lindong28 · 2015-10-18T20:13:28Z

Not sure why test failed.. It passed on my machine. Will make slight change and test again.

lindong28 · 2015-10-19T02:38:39Z

@ijuma @auradkar Thanks for taking time to review the patch. Can you see if the updated patch address your concern?

guozhangwang · 2015-10-19T05:56:51Z

What will happen if allCleanerCheckpoints does not contain the partition?

If allCleanerCheckpoints doesn't contain the partition, cleanerManager.allCleanerCheckpoints.get(TopicAndPartition(topic, part)).fold(true)(_ < offset) will return true and while loop continues.

I was originally concerned that if the partition is not included due to some bugs then we are doomed to wait for 1 minute in practice; but then I realize this function is only going to be used in testing, so this should be fine.

ijuma · 2015-10-19T21:26:03Z

LGTM

guozhangwang · 2015-10-19T21:44:08Z

Thanks @ijuma @lindong28 for the thorough discussion, LGTM.

MaximGurschi · 2016-05-24T11:47:45Z

Hi, would you expect this patch to work on Windows?
This assert keeps failing on Windows (kafka 0.9.0.2):

assertTrue("log cleaner should have processed up to offset " + firstDirty2, lastCleaned2 >= firstDirty2);

Errors are:
cleanerTest[0]: "log cleaner should have processed up to offset 597"
cleanerTest[1]: "log cleaner should have processed up to offset 599"
cleanerTest[2]: "log cleaner should have processed up to offset 599"
cleanerTest[3]: "log cleaner should have processed up to offset 599"

…che#327)

This should effectively purge LI commit - [LI-HOTFIX] Update jackson-databind from vulnerable version (apache#317) == Original upstream commit [76ca62a] == KAFKA-13775: CVE-2020-36518 - Upgrade jackson-databind to 2.12.6.1 (apache#11962) CVE-2020-36518 vulnerability affects jackson-databind (see GHSA-57j2-w4cx-62h2). Upgrading to jackson-databind version 2.12.6.1 addresses this CVE. Reviewers: Luke Chen <showuon@gmail.com>, Bruno Cadonna <cadonna@apache.org> Co-authored-by: Edwin <edwinhobor@gmail.com>

The function did the deadlock-preventing ordering on a wrong query. The job test was enhanced to check different request orders work correctly. `EnforceRetentionJob` already had the code for processing results of such a PG function, which was generalized to serve both.

KAFKA-2669; Fix LogCleanerIntegrationTest

39c356a

lindong28 force-pushed the KAFKA-2669 branch from 4e80bd9 to 39c356a Compare October 17, 2015 09:19

ijuma reviewed Oct 17, 2015
View reviewed changes

improve awaitCleaned as reviewers suggested

2616cad

auradkar reviewed Oct 17, 2015
View reviewed changes

add timeout for awaitCleaned

6cbb4a5

lindong28 force-pushed the KAFKA-2669 branch from 1641fdf to 6cbb4a5 Compare October 18, 2015 20:14

guozhangwang reviewed Oct 19, 2015
View reviewed changes

Dong Lin and others added 3 commits October 19, 2015 00:33

replace return with nested function

74cd5e7

minor change

0f4d1cf

address comments

69ffb13

asfgit closed this in 343db8a Oct 19, 2015

lindong28 deleted the KAFKA-2669 branch October 19, 2015 21:50

efeg added a commit to efeg/kafka that referenced this pull request Jan 29, 2020

Add logDir information to cluster model and kafka cluster state. (apa…

d8ea121

…che#327)

Uh oh!

Conversation

lindong28 commented Oct 17, 2015

Uh oh!

lindong28 commented Oct 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lindong28 commented Oct 17, 2015

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!

lindong28 commented Oct 18, 2015

Uh oh!

lindong28 commented Oct 19, 2015

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!

ijuma commented Oct 19, 2015

Uh oh!

guozhangwang commented Oct 19, 2015

Uh oh!

MaximGurschi commented May 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MaximGurschi commented May 24, 2016 •

edited

Loading