Create basePersistDirectory only when needed - #3530
Conversation
|
hrm...thinking back. That fix may not be desirable. |
|
@praveev sorry for the delay. Your hunch is right, we don't want to create new temp dirs on each call to getBasePersistDirectory. The other tricky thing about this is that sometimes, things call getBasePersistDirectory even if they don't actually need to make a base persist directory. One example where this happens is that KafkaTuningConfig gets defaults by doing So what we want for #3347 is a scheme that allows the things that actually write to getBasePersistDirectory to get the behavior they need (if they call it multiple times, they should get the same response) but for things that don't write to getBasePersistDirectory to avoid triggering needless directory creations. |
|
@gianm does using builder pattern serve this purpose? Unit test seem to pass. This ensures that those who want basePersistDirectory explicitly create it ahead of time. No creation is done in the getBasePersistDirectory() call |
There was a problem hiding this comment.
Thanks @praveev -- the builder stuff is nice.
Other than the line comments, one other thing: this patch looks like it works well for indexing service tasks, but doesn't work right for standalone realtime.
According to the docs, standalone realtime should use a temp dir inside java.io.tmpdir for basePersistDirectory if one isn't specified. This patch will change the default base to null rather than Files.createTempDir() which means RealtimePlumber will use the current working directory instead.
To fix this, add some code to RealtimePlumber or RealtimePlumberSchool that uses Files.createTempDir() as the basePersistDirectory if it's null.
| .withMaxRowsInMemory(1) | ||
| .withVersioningPolicy(new IntervalStartVersioningPolicy()) | ||
| .withRejectionPolicyFactory(new NoopRejectionPolicyFactory()) | ||
| .withBasePersistDirectory(Files.createTempDir()) |
There was a problem hiding this comment.
Since this is a test could you change this to use a TemporaryFolder? (The advantage is that cleanup is automatic when the test finishes, instead of leaving stuff in the user's temp dir.)
To use it just add this to your test class:
@Rule
public final TemporaryFolder temporaryFolder = new TemporaryFolder();And then when you need a tmp dir get it by calling temporaryFolder.newFolder().
There was a problem hiding this comment.
Similar comment for AppenderatorTester, although that can't have a @Rule of its own since it's not a test class. So for that one, the three test classes that call it should have a TemporaryFolder they use to generate a temp dir that they pass in to the AppenderatorTester constructor.
| { | ||
| // Cannot be a static because default basePersistDirectory is unique per-instance | ||
| final RealtimeTuningConfig defaults = RealtimeTuningConfig.makeDefaultTuningConfig(basePersistDirectory); | ||
| final RealtimeTuningConfig defaults = new RealtimeTuningConfig.Builder() |
There was a problem hiding this comment.
This isn't quite right, but flows from a mistake in the original code. KafkaTuningConfig shouldn't have a basePersistDirectory. It's not documented and it doesn't make sense. (Base persist directory for tasks is set by runtime properties and always overridden via withBasePersistDirectory; so it shouldn't be provided in the task.)
Just remove basePersistDirectory from the constructor and let it stay null here.
There was a problem hiding this comment.
hope i understood your comment right. i've updated the pr
|
@praveev are you able to pick this back up? |
|
@gianm I am currently on vacation, I won't be able to get back to this until mid-April. |
|
Got it, no worries, was just checking in. Looking forward to reviewing again when you get back 😄 |
# Resolved Conflicts: # indexing-service/src/test/java/io/druid/indexing/common/task/RealtimeIndexTaskTest.java # indexing-service/src/test/java/io/druid/indexing/common/task/TaskSerdeTest.java # indexing-service/src/test/java/io/druid/indexing/overlord/TaskLifecycleTest.java # server/src/main/java/io/druid/segment/indexing/RealtimeTuningConfig.java # server/src/test/java/io/druid/segment/realtime/RealtimeManagerTest.java # server/src/test/java/io/druid/segment/realtime/appenderator/AppenderatorPlumberTest.java # server/src/test/java/io/druid/segment/realtime/appenderator/AppenderatorTester.java # server/src/test/java/io/druid/segment/realtime/appenderator/DefaultOfflineAppenderatorFactoryTest.java # server/src/test/java/io/druid/segment/realtime/plumber/RealtimePlumberSchoolTest.java # server/src/test/java/io/druid/segment/realtime/plumber/SinkTest.java # services/src/test/java/io/druid/cli/validate/DruidJsonValidatorTest.java
|
@gianm Updated the PR. Not entirely sure if I understood your comment right, esp about the basePersistDirectory and KafkaTuningConfig. Please take a look and let me know. |
|
@praveev can we finish this? |
|
waiting on @gianm to review the updates |
|
Sorry @praveev, I totally missed this. If you are still interested in working on this patch, could you please resolve the conflicts and then we could take another look. |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@druid.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
6 participants
@gianm is this what you had in mind for #3347?
I'm not sure how to repro and test the random tempDir creation failure tho.