wernerdv commented on code in PR #13409:
URL: https://github.com/apache/ignite/pull/13409#discussion_r3703652314
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/metastorage/persistence/DistributedMetaStorageImpl.java:
##########
@@ -1318,56 +1305,50 @@ void clearHistoryCache() {
/**
* Notify listeners on node start. Even if there was no data restoring.
*
- * @param newData Data about which listeners should be notified.
+ * @param newDataKeys Data keys about which listeners should be notified.
+ * @param newDataVals Data values about which listeners should be notified.
*/
- private void notifyListenersBeforeReadyForWrite(
- DistributedMetaStorageKeyValuePair[] newData
- ) throws IgniteCheckedException {
+ private void notifyListenersBeforeReadyForWrite(String[] newDataKeys,
byte[][] newDataVals) throws IgniteCheckedException {
Review Comment:
Now `notifyListenersBeforeReadyForWrite` drops old-key removal notifications.
The merge logic in `notifyListenersBeforeReadyForWrite` was rewritten from a
two-pointer `while` loop over two sorted arrays to a `for (var oldE :
oldData.entrySet())` loop.
This introduces a correctness regression.
**Problem.** In the old code the `c > 0` branch (new key sorts before the
current old key) advanced only `newIdx` and kept the current `oldKey` in place,
so the same `oldKey` was re-compared against the subsequent new keys on the
next iteration. The new `for`-loop, however, unconditionally advances to the
next old entry on every iteration. Consequently, when the `c > 0` branch is
taken, the current `oldKey` is never compared against the remaining new keys,
and its removal notification (`c < 0` branch, old-only) is silently skipped.
Example: old cache `{A, M, Z}`, incoming full data `{A, B, Z}`:
- `A == A` → both (*modified*), `newIdx = 1`
- `M > B` → "B added", `newIdx = 2` ← **M is dropped here, never
re-compared with Z**
- `Z == Z` → both, `newIdx = 3`
Result: listeners get `{A: both, B: added, Z: both}` and are **never
notified that `M` was removed**. In the minimal case old `{B}`, new `{A, B}`
the key `B` is even misclassified as *added* instead of *modified*.
This path runs every time a node ingests a full distributed-metastorage
snapshot (join with insufficient history, or after a lag), so the
misclassification is user-visible.
**Fix.** Restore the two-pointer merge semantics with an explicit iterator
so the current old key is re-compared after each `c > 0` advance of `newIdx`:
```
Iterator<Map.Entry<String, byte[]>> oldIt =
oldData.entrySet().iterator();
Map.Entry<String, byte[]> oldE = oldIt.hasNext() ? oldIt.next() : null;
int newIdx = 0;
while (oldE != null && newIdx < newDataKeys.length) {
String oldKey = oldE.getKey();
byte[] oldValBytes = oldE.getValue();
String newKey = newDataKeys[newIdx];
byte[] newValBytes = newDataVals[newIdx];
int c = oldKey.compareTo(newKey);
if (c < 0) {
notifyListeners(oldKey, () -> unmarshal(marshaller,
oldValBytes), () -> null);
oldE = oldIt.hasNext() ? oldIt.next() : null;
}
else if (c > 0) {
notifyListeners(newKey, () -> null, () -> unmarshal(marshaller,
newValBytes));
++newIdx; // do NOT advance oldE — re-compare the same old key
next iteration
}
else {
notifyListeners(oldKey, () -> unmarshal(marshaller, oldValBytes),
() -> unmarshal(marshaller, newValBytes));
oldE = oldIt.hasNext() ? oldIt.next() : null;
++newIdx;
}
}
while (oldE != null) { // remaining old keys are removals
notifyListeners(oldE.getKey(), () -> unmarshal(marshaller,
oldE.getValue()), () -> null);
oldE = oldIt.hasNext() ? oldIt.next() : null;
}
for (; newIdx < newDataKeys.length; ++newIdx) // remaining new keys are
additions
notifyListeners(newDataKeys[newIdx], () -> null, () ->
unmarshal(marshaller, newDataVals[newIdx]));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
