You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/01/28 22:52:57 UTC
[GitHub] [samza] bkonold opened a new pull request #1261: SAMZA-2447:
Checkpoint dir removal should only search in valid store dirs
bkonold opened a new pull request #1261: SAMZA-2447: Checkpoint dir removal should only search in valid store dirs
URL: https://github.com/apache/samza/pull/1261
**Symptom:** NPE when attempting to remove old checkpoint directories.
**Cause:** Removal of old checkpoints assumes every file in the logged store base directory is also a directory, which may not be true if the user has configured it to a path that is not exclusive to store contents.
**Changes:** Check that the result of `listFiles` is non-null before attempting to delete.
**Tests:** Wrote a test to mimic the behavior of a non-directory file in the logged store base directory. Test fails without the patch, passes with it.
**API Changes:** None
**Upgrade Instructions:** None
**Usage Instructions:** None
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [samza] prateekm commented on a change in pull request #1261:
SAMZA-2447: Checkpoint dir removal should only search in valid store dirs
Posted by GitBox <gi...@apache.org>.
prateekm commented on a change in pull request #1261: SAMZA-2447: Checkpoint dir removal should only search in valid store dirs
URL: https://github.com/apache/samza/pull/1261#discussion_r372111920
##########
File path: samza-core/src/test/scala/org/apache/samza/storage/TestTransactionalStateTaskStorageManager.java
##########
@@ -492,6 +493,32 @@ public void testWriteChangelogOffsetFilesWithNoChangelogOffset() throws IOExcept
fail("Should have thrown an exception if no changelog offset found for checkpointed store");
}
+ @Test
+ public void testRemoveOldCheckpoints() {
Review comment:
Looks like this is testing the specific sub-case when listFiles returns null. If so, let's rename method to reflect that (e.g. testRemoveOldCheckpointsWhenXYZ
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [samza] bkonold commented on a change in pull request #1261:
SAMZA-2447: Checkpoint dir removal should only search in valid store dirs
Posted by GitBox <gi...@apache.org>.
bkonold commented on a change in pull request #1261: SAMZA-2447: Checkpoint dir removal should only search in valid store dirs
URL: https://github.com/apache/samza/pull/1261#discussion_r372118764
##########
File path: samza-core/src/test/scala/org/apache/samza/storage/TestTransactionalStateTaskStorageManager.java
##########
@@ -492,6 +493,32 @@ public void testWriteChangelogOffsetFilesWithNoChangelogOffset() throws IOExcept
fail("Should have thrown an exception if no changelog offset found for checkpointed store");
}
+ @Test
+ public void testRemoveOldCheckpoints() {
Review comment:
done
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [samza] prateekm merged pull request #1261: SAMZA-2447: Checkpoint
dir removal should only search in valid store dirs
Posted by GitBox <gi...@apache.org>.
prateekm merged pull request #1261: SAMZA-2447: Checkpoint dir removal should only search in valid store dirs
URL: https://github.com/apache/samza/pull/1261
----------------------------------------------------------------
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
With regards,
Apache Git Services