-
Notifications
You must be signed in to change notification settings - Fork 76
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
Fix Log Replication Deletions #7234
Fix Log Replication Deletions #7234
Conversation
6d17ed6
to
e3e3ada
Compare
@Utkarsh-pro #7235 will this PR verify these changes? |
e3e3ada
to
30f4c19
Compare
// if there is already a candidate for the same key, then we have a conflict | ||
if (time_map[key]) { | ||
const conflict_candidate = time_map[key]; | ||
conflict_candidate.action = 'conflict'; |
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.
should we avoid here versioning enabled buckets? this "squash" will not create the same versions as we have in the source bucket
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, I had questions about versioned objects when I picked up the task. Unfortunately, we don't yet have a "doc" yet (not that I know of) which clarifies how we deal with the cases like:
- Source has versioning destination doesn't.
- Destination has versioning source doesn't.
- Both have versioning.
eg. What to do with delete markers when destination doesn't have versioning? Not that it can't be done, just that we don't have it documented yet.
This might be a broader discussion I think, that's why the implementation currently does not do anything special when it comes to versioned objects.
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.
Alright, for creating a better user experience I think that we should document it
30f4c19
to
3d8652c
Compare
Signed-off-by: Ben <belimele@redhat.com>
|
||
// if there is already a candidate for the same key, then we have a conflict | ||
if (logs_for_key_time_map.has(log.time)) { | ||
// TODO: Object versioning will raise false alarms here |
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.
please open a github issue for that, it's a real gap the we should address
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.
@romayalon, opened an issue here: #7252
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.
perfect, thanks!
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
3d8652c
to
e822e4c
Compare
Signed-off-by: Ben <belimele@redhat.com>
Signed-off-by: Ben <belimele@redhat.com>
Signed-off-by: Ben <belimele@redhat.com>
Signed-off-by: Ben <belimele@redhat.com>
Explain the changes
This PR ensures that even if multiple actions are performed on an object, we process all of the actions instead of processing only the last one. Processing only the last action causes issues in the scenario when the logs are not chronologically ordered, for example:
DELETE /cat.jpeg
thenPUT /cat.jpeg
, when these 2 entries are part of the same candidate batch, NooBaa would just process the lastPUT
which means that theDELETE
will never be processed.PS: Added JS Docs to have the static analysis "safety net".
Issues: Fixed #xxx / Gap #xxx
This issue was discovered by Sagi (QE) during happy path testing.