ARSN-377 correctly handle null keys with common prefix #2192

jonathan-gramain · 2023-12-13T02:23:32Z

When encountering a null key, check for its common prefix before including it in either the Versions array or CommonPrefixes array, instead of always including it in the Versions array.

This commit refactors how DelimiterVersions works with null keys slightly: the null key is now inserted at its correct ordered position by the top-level filter() method, and the state machine handlers only have to deal with sorted versions. Previously the individual handlers would have to deal with the null key positioning themselves resulting in more complex state management.

bert-e · 2023-12-13T02:23:35Z

Hello jonathan-gramain,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2023-12-13T02:23:40Z

Incorrect fix version

The Fix Version/s in issue ARSN-377 contains:

7.70.15

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

7.70.15
8.1.116

Please check the Fix Version/s of ARSN-377, or the target
branch of this pull request.

jonathan-gramain · 2023-12-13T02:25:33Z

ping

bert-e · 2023-12-13T02:25:41Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

jonathan-gramain · 2023-12-13T02:25:49Z

/create_integration_branches

bert-e · 2023-12-13T02:25:57Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes with contents from bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes
and development/8.1.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes origin/development/8.1
 $ git merge origin/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes

The following options are set: create_integration_branches

bert-e · 2023-12-13T02:31:56Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_integration_branches

KillianG · 2023-12-14T07:30:48Z

tests/unit/algos/list/delimiterVersions.spec.js

+    { key: 'notes/summer/4.txt', value: fooDM, versionId: 'foo' },
+    { key: 'notes/summer/4.txt', value: quxDM, versionId: 'qux' },
+    { key: 'notes/summer/444.txt', value: fooDM, versionId: 'foo' },
+    { key: 'notes/summer/44444.txt', value: fooDM, versionId: 'foo' },


I feel like these 2 arrays can be simplified into only one, as receivedData is most likely just using dataVersioned array values and adds versionId field.. What do you think ?

It's actually not an exact 1-1, it depends on the keys, for example you can have a duplicate master/version key, or not (depending if it's V0 or V1 or if it's a current/noncurrent version, or delete marker). Hence I think it's best to keep them separate. There are actually 3 different arrays if you account for the V0 and V1 data.

KillianG · 2023-12-14T07:31:31Z

Have you tried running zenko tests and cloudserver tests to be sure we don't break anything with that change ?

jonathan-gramain · 2023-12-14T19:01:34Z

@KillianG

Have you tried running zenko tests and cloudserver tests to be sure we don't break anything with that change ?

I haven't, and it's a good idea, I will do. I ran tests on Metadata to make sure it fixes the case described in the V1 test plan, but since the risk of regression is not negligible I think running the automated test suites beforehand (including S3C Integration) is useful.

Add version IDs to delete marker metadata

When encountering a null key, check for its common prefix before including it in either the Versions array or CommonPrefixes array, instead of always including it in the Versions array. This commit refactors how `DelimiterVersions` works with null keys slightly: the null key is now inserted at its correct ordered position by the top-level `filter()` method, and the state machine handlers only have to deal with sorted versions. Previously the individual handlers would have to deal with the null key positioning themselves resulting in more complex state management.

bert-e · 2023-12-14T22:12:49Z

History mismatch

Merge commit #0e19a1e7f42ea5f40f287480a530db64dba45ab7 on the integration branch
w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes is merging a branch which is neither the current
branch bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes nor the development branch
development/8.1.

It is likely due to a rebase of the branch bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

The following options are set: create_integration_branches

jonathan-gramain · 2023-12-14T22:13:52Z

/reset

bert-e · 2023-12-14T22:14:00Z

Reset complete

I have successfully deleted this pull request's integration branches.

The following options are set: create_integration_branches

bert-e · 2023-12-14T22:14:10Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes with contents from bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes
and development/8.1.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes origin/development/8.1
 $ git merge origin/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes

The following options are set: create_integration_branches

bert-e · 2023-12-14T22:17:17Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_integration_branches

jonathan-gramain · 2023-12-14T22:46:24Z

@KillianG When running the Metadata and Cloudserver tests I found an issue with the DelimiterNonCurrent class that derives from DelimiterVersions and had to be modified. I fixed it, added a new unit test and re-ran the Metadata/Cloudserver tests, should be good now.

bert-e · 2023-12-14T22:52:56Z

History mismatch

Merge commit #f7144b9c165ed91d831157d03c557401137cdef0 on the integration branch
w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes is merging a branch which is neither the current
branch bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes nor the development branch
development/8.1.

It is likely due to a rebase of the branch bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

The following options are set: create_integration_branches

jonathan-gramain · 2023-12-14T22:53:11Z

/reset

bert-e · 2023-12-14T22:53:18Z

Reset complete

I have successfully deleted this pull request's integration branches.

The following options are set: create_integration_branches

bert-e · 2023-12-14T22:53:28Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes with contents from bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes
and development/8.1.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes origin/development/8.1
 $ git merge origin/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.1/bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes

The following options are set: create_integration_branches

bert-e · 2023-12-14T22:57:36Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_integration_branches

jonathan-gramain · 2023-12-20T18:20:12Z

/approve

bert-e · 2023-12-20T18:20:25Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/7.70
✔️ development/8.1

The following branches will NOT be impacted:

development/6.4
development/7.10
development/7.4

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve, create_integration_branches

bert-e · 2023-12-20T18:20:37Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/7.70
✔️ development/8.1

The following branches have NOT changed:

development/6.4
development/7.10
development/7.4

Please check the status of the associated issue ARSN-377.

Goodbye jonathan-gramain.

jonathan-gramain requested review from nicolas2bert and tmacro December 13, 2023 02:31

jonathan-gramain requested a review from KillianG December 13, 2023 21:23

KillianG reviewed Dec 14, 2023

View reviewed changes

jonathan-gramain added 2 commits December 14, 2023 14:12

ARSN-377 improve versioned listing test

206f14b

Add version IDs to delete marker metadata

jonathan-gramain force-pushed the bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes branch from ac18220 to f7144b9 Compare December 14, 2023 22:12

jonathan-gramain added 2 commits December 14, 2023 14:51

ARSN-377 fix DelimiterNonCurrent and add a unit test

063a2fb

ARSN-377 bump arsenal version

d08a267

jonathan-gramain force-pushed the bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes branch from f7144b9 to d08a267 Compare December 14, 2023 22:52

nicolas2bert approved these changes Dec 19, 2023

View reviewed changes

KillianG approved these changes Dec 20, 2023

View reviewed changes

bert-e merged commit d08a267 into development/7.70 Dec 20, 2023
7 checks passed

bert-e deleted the bugfix/ARSN-377-v1NullKeyDeleteMarkerNotInCommonPrefixes branch December 20, 2023 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARSN-377 correctly handle null keys with common prefix #2192

ARSN-377 correctly handle null keys with common prefix #2192

jonathan-gramain commented Dec 13, 2023

bert-e commented Dec 13, 2023

bert-e commented Dec 13, 2023

jonathan-gramain commented Dec 13, 2023

bert-e commented Dec 13, 2023

jonathan-gramain commented Dec 13, 2023

bert-e commented Dec 13, 2023

bert-e commented Dec 13, 2023

KillianG Dec 14, 2023

jonathan-gramain Dec 14, 2023

KillianG commented Dec 14, 2023

jonathan-gramain commented Dec 14, 2023

bert-e commented Dec 14, 2023

jonathan-gramain commented Dec 14, 2023

bert-e commented Dec 14, 2023

bert-e commented Dec 14, 2023

bert-e commented Dec 14, 2023

jonathan-gramain commented Dec 14, 2023

bert-e commented Dec 14, 2023

jonathan-gramain commented Dec 14, 2023

bert-e commented Dec 14, 2023

bert-e commented Dec 14, 2023

bert-e commented Dec 14, 2023

jonathan-gramain commented Dec 20, 2023

bert-e commented Dec 20, 2023

bert-e commented Dec 20, 2023

ARSN-377 correctly handle null keys with common prefix #2192

ARSN-377 correctly handle null keys with common prefix #2192

Conversation

jonathan-gramain commented Dec 13, 2023

bert-e commented Dec 13, 2023

Hello jonathan-gramain,

bert-e commented Dec 13, 2023

Incorrect fix version

jonathan-gramain commented Dec 13, 2023

bert-e commented Dec 13, 2023

Request integration branches

jonathan-gramain commented Dec 13, 2023

bert-e commented Dec 13, 2023

Conflict

bert-e commented Dec 13, 2023

Waiting for approval

KillianG Dec 14, 2023

Choose a reason for hiding this comment

jonathan-gramain Dec 14, 2023

Choose a reason for hiding this comment

KillianG commented Dec 14, 2023

jonathan-gramain commented Dec 14, 2023

bert-e commented Dec 14, 2023

History mismatch

jonathan-gramain commented Dec 14, 2023

bert-e commented Dec 14, 2023

Reset complete

bert-e commented Dec 14, 2023

Conflict

bert-e commented Dec 14, 2023

Waiting for approval

jonathan-gramain commented Dec 14, 2023

bert-e commented Dec 14, 2023

History mismatch

jonathan-gramain commented Dec 14, 2023

bert-e commented Dec 14, 2023

Reset complete

bert-e commented Dec 14, 2023

Conflict

bert-e commented Dec 14, 2023

Waiting for approval

jonathan-gramain commented Dec 20, 2023

bert-e commented Dec 20, 2023

In the queue

bert-e commented Dec 20, 2023