-
Notifications
You must be signed in to change notification settings - Fork 336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding internal config to retain non-logged stores on container start #1229
base: master
Are you sure you want to change the base?
Adding internal config to retain non-logged stores on container start #1229
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these changes.
static final String CLEAN_LOGGED_STOREDIRS_ON_START = STORE_PREFIX + "%s.clean.on.container.start"; | ||
|
||
// Internal config to clean storeDirs of a logged store on container start. This is used to benchmark bootstrap performance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/s/logged/non-logged/
static final String CLEAN_LOGGED_STOREDIRS_ON_START = STORE_PREFIX + "%s.clean.on.container.start"; | ||
|
||
// Internal config to clean storeDirs of a logged store on container start. This is used to benchmark bootstrap performance. | ||
static final String RETAIN_NONLOGGED_STOREDIRS_ON_START = STORE_PREFIX + "%s.retain.on.container.start"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we name this config to reflect the non-logged only case ? %s.retain.nonlogged.on.container.start
@@ -132,7 +132,7 @@ private void cleanBaseDirsAndReadOffsetFiles() { | |||
storageManagerUtil.getTaskStoreDir(nonLoggedStoreBaseDirectory, storeName, taskModel.getTaskName(), taskModel.getTaskMode()); | |||
LOG.info("Got non logged storage partition directory as " + nonLoggedStorePartitionDir.toPath().toString()); | |||
|
|||
if (nonLoggedStorePartitionDir.exists()) { | |||
if (nonLoggedStorePartitionDir.exists() || !storageConfig.getRetainNonloggedStoreDirsOnStart(storeName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update doc string to indicate that deletion is conditional.
@rmatharu please use PR description format. |
I'd prefer to not introduce force cleaning optional internal config for more combination of our stores properties (we already have one for logged store and now we have one for non-logged store). It makes it hard to follow up all the potential branching. If we still need this, can we consolidate all the flags to one configuration? Also, do we have other use cases for this outside experimental purpose? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a JIRA ticket and some description about this?
What do you need this for which can't be achieved by using a logged store? It is dangerous to keep non-logged stores around, because they could have stale and incomplete data.
@@ -132,7 +132,7 @@ private void cleanBaseDirsAndReadOffsetFiles() { | |||
storageManagerUtil.getTaskStoreDir(nonLoggedStoreBaseDirectory, storeName, taskModel.getTaskName(), taskModel.getTaskMode()); | |||
LOG.info("Got non logged storage partition directory as " + nonLoggedStorePartitionDir.toPath().toString()); | |||
|
|||
if (nonLoggedStorePartitionDir.exists()) { | |||
if (nonLoggedStorePartitionDir.exists() || !storageConfig.getRetainNonloggedStoreDirsOnStart(storeName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be &&
instead of ||
?
Is this PR still needed? Lets close it if its not. |
No description provided.