-
Notifications
You must be signed in to change notification settings - Fork 54
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
Removing an operation with mv command does not remove the operation #1978
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.
Removing an operation file using the mv command does not remove it from the list.
The reason for this is when a file is moved, then the file modified event gets generated and this will be considered as an add operation and tries to access the file and fails to read the file instead of removing the operation from the list.
I don't understand the issue. Do you mean that the Remove
event is not sent to the mapper? If so, what needs to be fix is the |inotify
support.
Ok(op) => ops.add_operation(op), | ||
Err(e) => { | ||
if e.to_string().eq("No such file or directory (os error 2)") { | ||
ops.remove_operation(&message.operation_name); |
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.
I just noticed that the remove_operation
is buggy since the beginning.
When an operation is removed, one needs not only to remove it from the operations
vector, one must also clear and rebuild the operations_by_trigger
map. It's not enough to remove the deleted entry in that map, because this map points to indexes in the operations
vector.
The better would be to simplify the struct Operations
and use only a HashMap
.
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.
Agree, It is better to have a HashMap
for operation.
Yes, when a file is removed from the |
Do you mean to say that we need to fix it in our
|
So that is the bug to fix.
It's more than likely that the bug is in our fs-notify wrapper. |
How about a simpler approach where we don't differentiate between |
For me, it makes sense as there will not be many operations to be loaded and the change is not that frequent. @reubenmiller @didier-wenzek what do you guys think about Albin's suggestion? |
add_or_remove_operation(message, &mut self.operations)?; | ||
// Re populate the operations irrespective add/remove/modify event | ||
self.operations = get_operations(message.ops_dir.clone())?; | ||
dbg!(&self.operations); |
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.
Don't forget to remove that before the final commit.
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.
Still to be removed ;-)
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.
done
Robot Results
Passed Tests
|
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.
I'm okay to reload the whole operation set when there is a change. However, the Operations
data structure must be revised. The current version is wrongly using a hashmap as a vector.
77951e2
to
5a175a9
Compare
5a175a9
to
21429a2
Compare
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
21429a2
to
ae373d2
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.
Approved
Proposed changes
Issue:
Removing an operation file using the
mv
command does not remove it from the tedge-mapper's operations list.The reason for this is when a file is moved, a
file modified
event gets generated and this will be considered as anadd
operation. So, the mapper tries to access the file and fails to read the file and add it to the list, instead of removing the operation from the list.Proposed solution:
file is modified
and amodified/Add
event is generated, and if the operation file does not exist then remove the operation from the list.-Types of changes
Paste Link to the issue
#1974
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments