You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/07/10 23:56:35 UTC

[GitHub] [kafka] mjsax opened a new pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

mjsax opened a new pull request #9010:
URL: https://github.com/apache/kafka/pull/9010


   *More detailed description of your change,
   if necessary. The PR title and PR message become
   the squashed commit message, so use a separate
   comment to ping reviewers.*
   
   *Summary of testing strategy (including rationale)
   for the feature or bug fix. Unit and/or integration
   tests are expected for any behaviour change and
   system tests should be considered for larger changes.*
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] vvcephei commented on a change in pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #9010:
URL: https://github.com/apache/kafka/pull/9010#discussion_r453133360



##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -507,4 +510,51 @@ public void shouldLockGlobalStateDirectoryWhenDirectoryCreationDisabled() throws
         initializeStateDirectory(false);
         assertTrue(directory.lockGlobalState());
     }
+
+    @Test
+    public void shouldNotFailWhenCreatingTaskDirectoryInParallel() throws Exception {

Review comment:
       In retrospect, this makes sense. The fs operations probably take an order of magnitude (at least) more time than the difference in time between starting the two threads. Thanks for adding it!




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] mjsax commented on a change in pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9010:
URL: https://github.com/apache/kafka/pull/9010#discussion_r453134258



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -47,10 +47,10 @@
 public class StateDirectory {
 
     private static final Pattern PATH_NAME = Pattern.compile("\\d+_\\d+");
-
-    static final String LOCK_FILE_NAME = ".lock";
     private static final Logger log = LoggerFactory.getLogger(StateDirectory.class);
+    static final String LOCK_FILE_NAME = ".lock";
 
+    private final Object taskCreationLock = new Object();

Review comment:
       That's fair.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ableegoldman commented on pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9010:
URL: https://github.com/apache/kafka/pull/9010#issuecomment-656953025


   Oh nevermind, you literally just did. Ignore my last 🙂 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] mjsax commented on pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #9010:
URL: https://github.com/apache/kafka/pull/9010#issuecomment-656942987


   Call for review @ableegoldman @vvcephei 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] mjsax merged pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
mjsax merged pull request #9010:
URL: https://github.com/apache/kafka/pull/9010


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] mjsax commented on pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #9010:
URL: https://github.com/apache/kafka/pull/9010#issuecomment-657105545


   Two failures (J11 and J14) of
   ```
   org.apache.kafka.connect.mirror.MirrorConnectorsIntegrationTest.testOneWayReplicationWithAutorOffsetSync1
   ```
   
   J8 passed.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] ableegoldman commented on a change in pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9010:
URL: https://github.com/apache/kafka/pull/9010#discussion_r453132871



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -47,10 +47,10 @@
 public class StateDirectory {
 
     private static final Pattern PATH_NAME = Pattern.compile("\\d+_\\d+");
-
-    static final String LOCK_FILE_NAME = ".lock";
     private static final Logger log = LoggerFactory.getLogger(StateDirectory.class);
+    static final String LOCK_FILE_NAME = ".lock";
 
+    private final Object taskCreationLock = new Object();

Review comment:
       super nit: taskCreationLock --> taskDirCreationLock




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] vvcephei commented on a change in pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #9010:
URL: https://github.com/apache/kafka/pull/9010#discussion_r453132853



##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -507,4 +510,51 @@ public void shouldLockGlobalStateDirectoryWhenDirectoryCreationDisabled() throws
         initializeStateDirectory(false);
         assertTrue(directory.lockGlobalState());
     }
+
+    @Test
+    public void shouldNotFailWhenCreatingTaskDirectoryInParallel() throws Exception {

Review comment:
       Huh, neat!




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] mjsax commented on pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #9010:
URL: https://github.com/apache/kafka/pull/9010#issuecomment-657106852


   Merged to `trunk` and cherry-picked to `2.6`.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] vvcephei commented on a change in pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #9010:
URL: https://github.com/apache/kafka/pull/9010#discussion_r453133360



##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -507,4 +510,51 @@ public void shouldLockGlobalStateDirectoryWhenDirectoryCreationDisabled() throws
         initializeStateDirectory(false);
         assertTrue(directory.lockGlobalState());
     }
+
+    @Test
+    public void shouldNotFailWhenCreatingTaskDirectoryInParallel() throws Exception {

Review comment:
       In retrospect, this makes sense. The fs operations probably take an order of magnitude more time than the difference in time between starting the two threads. Thanks for adding it!




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] mjsax commented on a change in pull request #9010: KAFKA-10262: Ensure that creating task directory is thread safe

Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9010:
URL: https://github.com/apache/kafka/pull/9010#discussion_r453131515



##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -507,4 +510,51 @@ public void shouldLockGlobalStateDirectoryWhenDirectoryCreationDisabled() throws
         initializeStateDirectory(false);
         assertTrue(directory.lockGlobalState());
     }
+
+    @Test
+    public void shouldNotFailWhenCreatingTaskDirectoryInParallel() throws Exception {

Review comment:
       Without the fix, this test fails like 100% of the time for me. (Kinda surprising -- seems that the disk IO operations give us nice thread context switches so we hit the issue reliably.)




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org