Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rmatharu-zz
Copy link
Contributor

No description provided.

Copy link
Contributor

@abhishekshivanna abhishekshivanna left a 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.
Copy link
Contributor

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";
Copy link
Contributor

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)) {
Copy link
Contributor

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.

@prateekm
Copy link
Contributor

prateekm commented Dec 5, 2019

@rmatharu please use PR description format.

@mynameborat
Copy link
Contributor

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?

Copy link
Contributor

@cameronlee314 cameronlee314 left a 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)) {
Copy link
Contributor

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

@mynameborat
Copy link
Contributor

Is this PR still needed? Lets close it if its not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants