ft: ARSN-388 implement GapSet (caching of listing gaps)

[jonathan-gramain]

The GapSet class is intended for caching listing "gaps", which are contiguous series of current delete markers in buckets, although the semantics can allow for other uses in the future.

The end goal is to increase the performance of listings on V0 buckets when a lot of delete markers are present, as a temporary solution until buckets are migrated to V1 format.

This data structure is intented to be used by a GapCache instance, which implements specific caching semantics (to ensure consistency wrt. DB updates for example).

[bert-e]

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]

Incorrect fix version

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

• 7.70.23

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

• 7.70.23

• 8.1.122

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

[bert-e]

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.

[fredmnl]

Don't we risk deleting some data from the structure here? What about if the new gap inserted actually spans multiple known chained gaps? e.g.

NewGap: +1234+
GapSet: +123+456+789+

We would end up deleting the second gap in the GapSet and then go on to lose keys 5 and 6.

[jonathan-gramain]

You're right, I will add a lookup on the chained gap, and if it exists, return it instead of deleting it (it may still be merged with following gaps by setGap)

[jonathan-gramain]

Thanks for the feedback @fredmnl !

I updated the PR with new commits that should address your comments.

[jonathan-gramain]

You're right, I will add a lookup on the chained gap, and if it exists, return it instead of deleting it (it may still be merged with following gaps by setGap)

[fredmnl]

One question on parallelism, I'm still new to Node so maybe all potential race conditions are handled by the mono-threaded nature of node. Could we have the following situation for example:

Initial state of the storage is that there are delete markers on key 1 to 5. 

Listing operation: x starts listing  1 .. 2 .. 3 .. 4 .. 5 .. done, broadcast gap 1-5
Put operation:     x                         puts 2
GapSet:            x                         2 clears cache                    add 1-5 to cache

Basically a concurrent list and PUT operation will not update the data store in the right order. Is this possible?

[jonathan-gramain]

One question on parallelism, I'm still new to Node so maybe all potential race conditions are handled by the mono-threaded nature of node. Could we have the following situation for example:

Initial state of the storage is that there are delete markers on key 1 to 5. 

Listing operation: x starts listing  1 .. 2 .. 3 .. 4 .. 5 .. done, broadcast gap 1-5
Put operation:     x                         puts 2
GapSet:            x                         2 clears cache                    add 1-5 to cache

Basically a concurrent list and PUT operation will not update the data store in the right order. Is this possible?

Yes you're totally correct, there is a race condition that we need to address in some way. I started thinking about this yesterday and potential solutions to it. Updating the cache after every listed key would not be enough as we can always have updates on or between listed keys (i.e. in the inclusive range between two listed keys).

My current idea is that the GapSet class somehow needs to be aware of the gaps that are in construction from the listings in progress, and either ignore or invalidate calls to setGap when those gaps overlap with PUT/DELETE updates that occurred while this gap construction was happening. In other words, if GapSet.setGap(firstKey, lastKey) is aware of when the firstKey was initialized, it can know whether there were new updates in the range [firstKey, lastKey] when the gap is set and ignore the call in such case.

Another idea can be to track all listings in progress, and push invalidation to each listing in progress when removeOverlappingGaps is called. Basically considering that the gaps in construction are already part of the gap set (but not yet usable by other listings).

The funny thing is originally I though of an API for setGap that takes a gap object returned from a prior call, rather than a first and last key, but I was concerned about such race conditions (in the sense that the gap may have been invalidated in the meantime), and decided to switch to first/last strings instead. Using a gap object could actually provide a way to deal with those race conditions by storing some state in the gap objects, although I'm not sure of the details. It could be a raft sequence number (cseq) that would be compared against the recent updates, for example.

[jonathan-gramain]

So I came up with another approach to ensure atomicity, this PR should explain it: #2213

[fredmnl]

One thing that's still bugging me is that we are checking in _lookupOrCreateGap if there is a gap straddling newGap.firstKey and we are handling it depending on the weights. Yet, when we come to the gap that may be straddling newGap.lastKey we don't perform the same check. If I'm correct we could be extending a gap to an arbitrarily large weight if we are only extending it toward lower values (a workload that would delete objects with ever decreasing keys).

If all of this is correct, the logic could be summarized as:

• look for a gap straddling newGap.firstKey and make a decision as to whether we want to extend it, or split.
• look for a gap straddling newGap.lastKey and make a decision as to whether we want to extend it, or split (which may depend on the previous decision)
• with the boundaries handled, remove all of the gaps that are fully contained within newGap and insert newGap

What do you think?

[jonathan-gramain]

There is indeed the issue that you mentioned, it's probably what I observed too during my testing.

I am reworking this and should be able to finish on Monday, will let you know when it's done! I think the logic that you describe is good, there is some complexity in checking all cases and setting the weights accordingly but it should be achievable.

[jonathan-gramain]

I reworked this in a new commit

[jonathan-gramain]

/approve

[bert-e]

Conflict

A conflict has been raised during the creation of
integration branch w/8.1/feature/ARSN-388-gapSet with contents from feature/ARSN-388-gapSet
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/feature/ARSN-388-gapSet origin/development/8.1
 $ git merge origin/feature/ARSN-388-gapSet
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.1/feature/ARSN-388-gapSet

The following options are set: approve

[bert-e]

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

[bert-e]

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-388.

Goodbye jonathan-gramain.