KAFKA-6647: Do note delete the lock file while holding the lock by guozhangwang · Pull Request #8267 · apache/kafka

guozhangwang · 2020-03-10T23:13:53Z

Inside StateDirectory#cleanRemovedTasks, skip deleting the lock file (and hence the parent directory) until releasing the lock. And after the lock is released only go ahead and delete the parent directory if manualUserCall == true. That is, this is triggered from KafkaStreams#cleanUp and users are responsible to make sure that Streams instance is not started and hence there are no other threads trying to grab that lock.
As a result, during scheduled cleanup the corresponding task.dir would not be empty but be left with only the lock file, so effectively we still achieve the goal of releasing disk spaces. For callers of listTaskDirectories like KIP-441 (cc @ableegoldman to take a look) I've introduced a new listNonEmptyTaskDirectories which excludes such dummy task.dirs with only the lock file left.
Also fixed KAFKA-8999 along the way to expose the exception while traversing the directory.

Committer Checklist (excluded from commit message)

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

guozhangwang · 2020-03-10T23:15:05Z

-                    if (manualUserCall) {
-                        throw e;
-                    }
+                } catch (final OverlappingFileLockException | IOException e) {


This is a minor code clean up.

guozhangwang · 2020-03-10T23:16:14Z

+                    throw exc;
+                }
+
+                if (rootFile.toPath().equals(path)) {


This is a bit tricky: for the root file, we only consider deleting it if there's no specified skipping sub-files; otherwise we never try to delete since it would doom with DirectoryNotEmpty.

It would be good to avoid the unnecessary conversions in this method (rootFile.toPath, path.toFile). We can do some work at the start of the method to improve efficiency.

ableegoldman · 2020-03-11T00:42:48Z

+    private final Time time;
+    private final String appId;
    private final File stateDir;
    private final boolean createStateDirectory;


What do you think about renaming this, eg something like isStateful, hasStatefulTopology`, etc?

It's really confusing to reason about in its usage, for example why createStateDirectory => #lock returns true. You have to backtrack to where createStateDirectory is set to understand this. However, I think it's easy to reason about why isStateful => should create state directory.

Alternatively, what if we have a stateless version of the StateDirectory class that just stubs things where appropriate. Then we could get rid of this altogether. WDYT?

That makes sense, I will do the renaming.

ableegoldman · 2020-03-11T00:52:50Z

     * @return The list of all the existing local directories for stream tasks
     */
-    File[] listTaskDirectories() {
+    File[] lisAllTaskDirectories() {


Suggested change

File[] lisAllTaskDirectories() {

File[] listAllTaskDirectories() {

mjsax · 2020-03-11T00:55:33Z

@guozhangwang I opened a PR for something related some time ago: #4713

Does it overlap?

guozhangwang · 2020-03-11T00:59:59Z

@guozhangwang I opened a PR for something related some time ago: #4713

Does it overlap?

Good call.

My proposal is to skip trying to delete the lock file, which I think is essentially similar to yours (capture the exception and then skip).

…-not-delete-lock-filet

guozhangwang · 2020-03-12T23:11:42Z

I've finally able to re-produce the issue with NTFS using this example code: https://github.com/simplesteph/kafka-streams-course/blob/1.1.0/word-count/src/test/java/com/github/simplesteph/udemy/kafka/streams/WordCountAppTest.java

And with my PR now it is proven fixed:

I'm going to add more unit tests in my PR but the non-testing part should be ready to review now cc @mjsax @vvcephei

guozhangwang · 2020-03-12T23:14:00Z

Also cc @rodesai

vvcephei

Thanks, Guozhang!

Just a couple of questions, and I think Sophie’s question is a good one, too.

Otherwise, lgtm!

vvcephei · 2020-03-13T04:11:32Z


    @Test
-    public void shouldListAllTaskDirectories() {
+    public void shouldNotListNonEmptyTaskDirectories() {


Is the double negative intentional here?

Good call, will change.

vvcephei · 2020-03-13T04:16:48Z

-                    } catch (final IOException e) {
-                        log.error("{} Failed to release the state directory lock.", logPrefix());
+
+                        // for manual user call, stream threads are not running so it is safe to delete


This comment makes me wonder why we even bother with locking at all for manual calls. If we know there’s no app running, why not just delete the whole state directory and not bother with locks?

I thought about that too, and tried it out, it's just that based on manualUserCall to decide whether lock / unlock the block, we ended up with either much more code duplicates or clumsy and finer-grained if else condition, neither of which I like. On the other hand, since streams.cleanUp is usually a one-time thing compared with the periodic internal clean I think having two delete calls are okay to same some code duplication here.

…-not-delete-lock-file

guozhangwang · 2020-03-13T16:46:08Z

@vvcephei @ableegoldman addressed your comments, please take another look.

ableegoldman

We just merged PR 8246, can you rebase and make the TaskManager#tryToLockAllTaskDirectoriesleverage the newlistNonEmptyTaskDirectories` method?
Otherwise LGTM

guozhangwang · 2020-03-14T20:12:03Z

@ableegoldman Just clarifying on the releaseLockedUnassignedTaskDirectories side, it's okay to just iterate lockedTaskDirectories.iterator() and not stateDirectory.listNonEmptyTaskDirectories() right?

Also I renamed tryToLockAllTaskDirectories to tryToLockAllNonEmptyTaskDirectories.

@ableegoldman

1. Inside StateDirectory#cleanRemovedTasks, skip deleting the lock file (and hence the parent directory) until releasing the lock. And after the lock is released only go ahead and delete the parent directory if manualUserCall == true. That is, this is triggered from KafkaStreams#cleanUp and users are responsible to make sure that Streams instance is not started and hence there are no other threads trying to grab that lock. 2. As a result, during scheduled cleanup the corresponding task.dir would not be empty but be left with only the lock file, so effectively we still achieve the goal of releasing disk spaces. For callers of listTaskDirectories like KIP-441 (cc @ableegoldman to take a look) I've introduced a new listNonEmptyTaskDirectories which excludes such dummy task.dirs with only the lock file left. 3. Also fixed KAFKA-8999 along the way to expose the exception while traversing the directory. Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, John Roesler <vvcephei@apache.org>

guozhangwang · 2020-03-14T21:09:17Z

Merged to trunk and 2.5 after tests green locally.

* apache-github/trunk: (39 commits) MINOR: cleanup and add tests to StateDirectoryTest (apache#8304) HOTFIX: StateDirectoryTest should use Set instead of List (apache#8305) MINOR: Fix build and JavaDoc warnings (apache#8291) MINOR: Fix kafka.server.RequestQuotaTest missing new ApiKeys. (apache#8302) KAFKA-9712: Catch and handle exception thrown by reflections scanner (apache#8289) KAFKA-9670; Reduce allocations in Metadata Response preparation (apache#8236) MINOR: fix Scala 2.13 build error introduced in apache#8083 (apache#8301) MINOR: enforce non-negative invariant for checkpointed offsets (apache#8297) MINOR: comment apikey types in generated switch (apache#8201) MINOR: Fix typo in CreateTopicsResponse.json (apache#8300) KIP-546: Implement describeClientQuotas and alterClientQuotas. (apache#8083) KAFKA-6647: Do note delete the lock file while holding the lock (apache#8267) KAFKA-9677: Fix consumer fetch with small consume bandwidth quotas (apache#8290) KAFKA-9533: Fix JavaDocs of KStream.transformValues (apache#8298) MINOR: reuse pseudo-topic in FKJoin (apache#8296) KAFKA-6145: Pt 2. Include offset sums in subscription (apache#8246) KAFKA-9714; Eliminate unused reference to IBP in `TransactionStateManager` (apache#8293) KAFKA-9718; Don't log passwords for AlterConfigs in request logs (apache#8294) KAFKA-8768: DeleteRecords request/response automated protocol (apache#7957) KAFKA-9685: Solve Set concatenation perf issue in AclAuthorizer ...

SaberZA · 2020-03-25T15:56:44Z

Thank you so much for this fix!! Any idea when this will get into the next release? or how I could integrate the change sooner?

guozhangwang · 2020-03-25T16:06:29Z

It is merged into 2.5 and branch, so it will be at least in 2.5.1 and 2.6.0; if 2.5.0 RC0 gets vetoed and a new one is cut, then it would also be in 2.5.0 as well.

guozhangwang added 4 commits March 9, 2020 17:25

first try

0d3aceb

pass in LOCK_FILE_NAME

7e0a503

refactoring

fe25917

final fix

22844d0

guozhangwang commented Mar 10, 2020

View reviewed changes

ableegoldman reviewed Mar 11, 2020

View reviewed changes

Merge branch 'trunk' of https://github.com/apache/kafka into K6647-do…

83b7fbe

…-not-delete-lock-filet

ableegoldman mentioned this pull request Mar 12, 2020

KAFKA-6145: Pt 2. Include offset sums in subscription #8246

Merged

guozhangwang changed the title ~~KAFKA-6647: Do note delete the lock file while holding the lock [WIP]~~ KAFKA-6647: Do note delete the lock file while holding the lock Mar 13, 2020

unit tests

e9618b1

mjsax added the streams label Mar 13, 2020

vvcephei reviewed Mar 13, 2020

View reviewed changes

guozhangwang added 2 commits March 13, 2020 09:32

Merge branch 'trunk' of https://github.com/apache/kafka into K6647-do…

b963e3a

…-not-delete-lock-file

address github comments

9e31460

vvcephei approved these changes Mar 13, 2020

View reviewed changes

ableegoldman requested changes Mar 14, 2020

View reviewed changes

guozhangwang added 2 commits March 14, 2020 13:14

resolve conflicts from trunk

e55dd4f

fix unit tests and renaming

e769560

guozhangwang merged commit 605d55d into apache:trunk Mar 14, 2020

chia7712 mentioned this pull request Mar 16, 2020

Minor: fix flaky StateDirectoryTest.shouldOnlyListNonEmptyTaskDirecto… #8303

Closed

3 tasks

mjsax mentioned this pull request Mar 17, 2020

MINOR: fix KafkaStreams#cleanUp(): should throw and fail for lock conflict #4713

Closed

guozhangwang deleted the K6647-do-not-delete-lock-file branch April 25, 2020 00:00

	File[] lisAllTaskDirectories() {
	File[] listAllTaskDirectories() {

Conversation

guozhangwang commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mjsax commented Mar 11, 2020

Uh oh!

guozhangwang commented Mar 11, 2020

Uh oh!

guozhangwang commented Mar 12, 2020

Uh oh!

guozhangwang commented Mar 12, 2020

Uh oh!

vvcephei 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!

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!

guozhangwang commented Mar 13, 2020

Uh oh!

ableegoldman left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guozhangwang commented Mar 14, 2020

Uh oh!

SaberZA commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guozhangwang commented Mar 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

guozhangwang commented Mar 10, 2020 •

edited

Loading

guozhangwang commented Mar 14, 2020 •

edited

Loading

SaberZA commented Mar 25, 2020 •

edited

Loading