Skip to content
This repository has been archived by the owner on Jul 7, 2020. It is now read-only.

changed the file writing mode to avoid an empty minion.state wh… #268

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

satang500
Copy link
Contributor

@satang500 satang500 commented Sep 19, 2017

  • Changed file mode for minion.state to append
    ( except when minion starts (which is the same in the original code) )
  • added minion close before shutdown when TaskRunner run throws an exception

Question: There are two places where they use minionStateLock again
(run method in CommandTaskDeleteRunner and kickNextJob method in Minion)
even though writeState() is already locking via minionStateLock.
Since this is ReentrantLock, it's no harm, but wondering why they use this way.

@satang500 satang500 changed the title [WIP] changed the file writing mode to avoid an empty minion.state wh… changed the file writing mode to avoid an empty minion.state wh… Sep 25, 2017
@@ -581,10 +581,10 @@ private boolean updateTaskMeta(String jobID, File taskRoot) throws IOException {
}
}

void writeState() {
void writeState(boolean append) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is append the correct thing to do? Minion state is a JSON object. If you keep appending, wont that result in invalid json in the file? Have you tested that?

Copy link

@ranadessr ranadessr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other comment I have is that there should be logging that indicates minion.state file is empty. Currently, we find a minion did not start and then we have to look to see it was because the state file is empty

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

Successfully merging this pull request may close these issues.

None yet

2 participants