-
Notifications
You must be signed in to change notification settings - Fork 74
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
Fixes #15864: add an option to prevent backup of node policy at the end of policy generation #2561
Conversation
…nd of policy generation
Commit modified |
79ffe1a
to
0f12fd5
Compare
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.
Two formating changes, and a question :)
* Move the generated promises from the new folder to their final folder, backuping previous promises in the way | ||
* @param folder : (Container identifier, (base folder, new folder of the policies, backup folder of the policies) ) | ||
* Move the generated policies from the new folder to their final folder, backuping previous policies in the way | ||
* if backupPoliciesAtEndOfGeneration is true, otherwise removing preivous policies |
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/preivous/previous
logger.trace("Backuping old promises from %s to %s ".format(baseFolder, backupFolder)) | ||
backupNodeFolder(baseFolder, backupFolder) | ||
if (backupPoliciesAtEndOfGeneration) { | ||
logger.trace("Backuping old policies from %s to %s ".format(baseFolder, backupFolder)) |
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.
could you change it to: s"Backuping old policies from ${baseFolder} to ${backupFolder}"
for consistency ?
backupNodeFolder(baseFolder, backupFolder) | ||
} else { | ||
logger.trace(s"Deleting old policies in ${baseFolder}") | ||
deleteNodeFolder(baseFolder) |
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.
shouldn't we also delete backupFolder if it exits from a previous generation with a different backup strategy?
Else, people may become confuse when/if they find back the olds generated files at some point.
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.
yes, that's a good idea
here is something odd here: with the delete
with the move
|
delete: [2019-10-28 23:24:06] DEBUG policy.generation - Policies moved to their final position in 354937 ms |
/** | ||
* Delete the previous node policies folder | ||
*/ | ||
private[this] def deleteNodeFolder(nodeFolder: String): Unit = { |
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.
actually, we could skip it as moveNewNodeFolder does the deletion of dest if it exists
So, what do we do with that one? Close it as "not a good idea finaly?" |
We will close that PR for now, until we have better understanding of the underlying perf. |
https://issues.rudder.io/issues/15864