Improve maintainability for kvstoreScan#3588
Conversation
rainsupreme
left a comment
There was a problem hiding this comment.
Looks good to me!
While this is a worthy simplification, my understanding is that this is also part of the forkless work, as your plan is to use kvstore scan for the order that items are traversed, and it will need to understand whether incoming changes are before or after the cursor position.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #3588 +/- ##
============================================
+ Coverage 76.66% 76.67% +0.01%
============================================
Files 162 162
Lines 80644 80643 -1
============================================
+ Hits 61823 61835 +12
+ Misses 18821 18808 -13
🚀 New features to boost your workflow:
|
zuiderkwast
left a comment
There was a problem hiding this comment.
This changes the cursor values so it affects cursor compatibilty between versions.
We need to bump the fingerprint in the CLUSTERSLOTS cursor introduced in #2934.
It breaks SCANs that are performed over a period involving a failover in a mixed-versions cluster such as during a rolling upgrade, even if the nodes are configured with a fixed hash seed as introduced in #2608.
Given these problems, I'm not sure it's a good idea to accept this change.
murphyjacob4
left a comment
There was a problem hiding this comment.
It's more readable to me now
|
We need cross-version tests for the cross-node SCAN features mentioned above. |
Good point on CLUSTERSCAN I specifically remember in the CLUSTERSCAN design we wanted to encode the "memory layout version" to prevent the need for the cursor to maintain cross version stability. I think we should be okay making the cursor reset across versions, it wouldn't be breaking, just less efficient. Otherwise, it is very restrictive. We should be okay re-fingerprinting on each minor version, if needed. |
Yeah, we just need a test to catch when the cursor or memory layout changes. The test can be updated whenever we make those changes to make it pass |
We wanted to encode so that we could change it as needed, I don't think readability is reason to change it. I agree with Viktor that I would rather not take this. Also, agree on the cross version scan test. We already have cross version testing, we can add SCAN to that. |
There was a problem hiding this comment.
just suggestion not different from whats already said:
Add to CLUSTERSCAN fingerprint
as introduced in #2934, the fingerprint in clusterscanFingerprint() only hashes the see, we can incorporate a cursor layout version so the mismatch is detected and the scan can restarts cleanly:
in src/cluster.c
#define CURSOR_LAYOUT_VERSION 1
static const char *clusterscanFingerprint(void) {
.....
uint64_t hash = wangHash64(seed[0] ^ seed[1] ^ CURSOR_LAYOUT_VERSION);
.....
}
like @zuiderkwast @murphyjacob4 mentioned.
WDYT?
Yeah this was one thing I was worried about with the CLUSTERSCAN feature - I don't like us adding additional contracts (whether implicit, best effort, or otherwise) to the stability of the cursor over versions. I would almost prefer that we always break it over minor versions just so nobody takes a dependency on this and then gets broken when a bump does happen. Also CLUSTERSCAN hasn't GA'd yet - so we could always merge this into 9.1 before GA and it should be fine? |
I agree the contracts should be explicit.
Good point that CLUSTERSCAN isn't GA yet. Nether is We discussed at some point in the past that we have two spare bits in the cursor (we only need 14 bits for the cluster slot, not 16) and that we can use these as a version of some kind, but that'd be internal and clients shouldn't be aware of the cursor representation. Perhaps we should just explicitly mention that it's not stable accross version in its documentation in valkey.conf, which currently doesn't cover versions: Another thing I remember we discussed when we introduced kvstore is that we can avoid returning very large numbers for the cursor. IIRC, that's why we didn't shift it in standalone mode and also why we put the slot in the lower bits instead of in the highest 16 bits. A small hash table will have a relatively small cursor. |
The way this is documented - I expect people would think it is stable across versions. In fact, if there is no "fingerprint" in We should explicitly document it is only stable if all instances are running the same version. We should also point out
Yeah... cursor size seems like a non-issue. In theory we save a few bytes on the ascii encoding but if you are anyways doing SCAN, it would be a very small amount of overhead.
I don't care too much about which way we go. I do think 48|16 is simpler. But the variable 64|0 and 50|14 representation is manageable. Either way, it is good to hash out the consequences of |
I feel like we (and by we, it's probably me) did a poor job explaining why a stable SCAN is important. It's not for the |
|
What did we do when we switched from dict to hashtable in kvstore and the datatypes? I don't remember discussing what happens to scans when a failover happens. Maybe we already have incompatibility between those versions? |
This change only affects kvstore scans though, i.e. just the scan command, so it wouldn't affect hscan and the other datatype scans, right? they use a simple un-shifted hashtable cursor afaik |
|
Certainly interesting discussion here. At this point, I'm of the mind that I can correct the readability issue - but leave the encoding as is. One thing that we should start to think about is that we can consider decoupling internal cursors from the external values used by client commands. For example, if we're worried about the cursor value being too large, and want to remove extra bits, this can be done by |
Signed-off-by: Jim Brunner <brunnerj@amazon.com>
| int skip = !ht || (skip_cb && skip_cb(ht)) || kvstoreIsImporting(kvs, didx); | ||
| if (!skip) { | ||
| next_cursor = hashtableScan(ht, cursor, hashtableScanToKvstoreScanCallback, &cb_data); | ||
| ht_cursor = hashtableScan(ht, ht_cursor, hashtableScanToKvstoreScanCallback, &cb_data); |
There was a problem hiding this comment.
Distinguishing between current cursor and next cursor is clearer. Removing the distinction make the same variable mean different things above and below this line.
There was a problem hiding this comment.
This is just an increment of the ht_cursor. The meaning remains the same... it's the current cursor both above and below. I think most readers would find this more intuitive. There's no need to maintain a variable for the old cursor value, and having 2 variables provides opportunity for mistake.
There was a problem hiding this comment.
They are different values. They represent the cursor at different points in time.
In functional languages where variables are bound once and values are immutable (Haskell, ML, Erlang, Elixir, PureScript(?)), the single-assignement is considered to give great clarity. In math text, you never see x = x + 1. Instead you see xn+1 = xn + 1.
Now, C isn't a functional language like that, but it's possible to write in different styles. Here, the style with different variables for the old and the new cursor is used and there is no reason to change it. It's subjective, and when it is, we don't change it.
|
@murphyjacob4 says:
@zuiderkwast says:
I agree that with the bit representation unchanged, this is just about readability. It eliminates the code smell of in-place conversion of the cursor format, and using the same variable for ht_cursor and kvs_cursor interchangeably. From a code complexity perspective, it eliminates more complex in/out parameters to the conversion routines. It clarifies naming (matter of opinion) and slightly improves the IF logic around the check for But ultimately, it's just about readability/maintainability. At this point, we should either merge or close. |
This is true. Feel free to merge it. It's been approved by multiple maintainers. I don't mind. (I just didn't think it was that important). |
The cursor in kvstore is composed of a hashtable cursor + an index of the hashtable within the kvstore. 48 bits are reserved for the hashtable cursor, and UP TO 16 bits are used for the hashtable index. The hashtable cursor is shifted by the number of bits actually needed for the hashtable index in a given kvstore (0-16).
This update shifts by a constant 16 bits. This eliminates variability in format. It also simplifies the cursor access functions as they no longer need to reference the specific kvstore in order to encode/decode the cursor. Reduced parameters and eliminated in/out parameters to cursor encode/decode functions.After discussion about coupling and potential impact with the SCAN command(s), reduced the scope of this change to just addressing readability around the cursor conversions.
Updated
kvstoreScanto more clearly show when a kvstore cursor was used vs. a hashtable cursor, rather than modifying it in-place in the same variable.