Conversation
1c223e5 to
d774da1
Compare
|
@elastic/beats Would be good to hear your thoughts on this change. |
There was a problem hiding this comment.
Should we be more strict here and check vs os.PathError?
There was a problem hiding this comment.
Good idea. What should we do in all other cases? Log an error and not remove the state?
There was a problem hiding this comment.
Yea that's probably the best we can do. os.PathError is all that can return right now but you never now..
There was a problem hiding this comment.
We could return here and check the error in the caller, and also remove the else
There was a problem hiding this comment.
I changed it to return directly if state is not finished but I kept the error handling local.
Previously if a file could be found under the same name as a state, the state was not removed. But this could have been also an other file. With this change also the file itself is compared and if it is not the same file, the state will be removed. This has the affect that in case a file is renamed after monitoring the file finished, the state will be removed. In most cases this should not have any side affect. The positive effect of this change is that there will be less left over states in the registry. Closes elastic#3789
d774da1 to
f8f540b
Compare
ruflin
left a comment
There was a problem hiding this comment.
review changes implemented and rebase on master
There was a problem hiding this comment.
I changed it to return directly if state is not finished but I kept the error handling local.
Previously if a file could be found under the same name as a state, the state was not removed. But this could have been also an other file. With this change also the file itself is compared and if it is not the same file, the state will be removed. This has the affect that in case a file is renamed after monitoring the file finished, the state will be removed. In most cases this should not have any side affect.
The positive effect of this change is that there will be less left over states in the registry.
Closes elastic#3789