bitflicker64 commented on code in PR #3382:
URL: https://github.com/apache/hugegraph/pull/3382#discussion_r4328882782
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/KvService.java:
##########
@@ -297,18 +298,65 @@ public void clearTTLData() {
byte[] ttlStartKey = getTTLStoreKey("", 0);
byte[] ttlEndKey = getTTLStoreKey("", System.currentTimeMillis());
List<KV> kvList = meta.scanRange(ttlStartKey, ttlEndKey);
+ // A marker is K@T@<time>@<key>: the key starts after the
delimiter that ends the time
+ int timeStart = (KV_PREFIX_DELIMITER + TTL_PREFIX +
KV_DELIMITER).length();
for (KV kv : kvList) {
String key = new String(kv.getKey());
- int index = key.indexOf(KV_DELIMITER, 2);
+ int index = key.indexOf(KV_DELIMITER, timeStart);
String delKey = key.substring(index + 1);
- delete(delKey);
- meta.remove(kv.getKey());
+ byte[] stored = meta.getOne(getStoreKey(delKey));
+ removeExpired(delKey,
key.substring(KV_PREFIX_DELIMITER.length()), stored);
}
} catch (Exception e) {
log.error("clear ttl data with error :", e);
}
}
+ /**
+ * Deletes a key whose TTL marker expired, and the marker, while the key
still holds the
+ * value the marker names. The value the cleanup read must carry the
marker's expiry, so a
+ * value written after the marker (with another TTL, or none) is kept and
only the stale
+ * marker goes. Right before the delete, the key must still hold the bytes
the cleanup read:
+ * a renewal writes another expiry, so its bytes differ even for the same
string. The check
+ * and the delete are separate store operations, so a renewal that lands
between them is
+ * still deleted; binding them in one raft entry needs the txn RPC of
#3376.
+ *
+ * @param key the key, without the K@ prefix
+ * @param marker the marker key, without the K@ prefix: T@<time>@<key>
+ * @param stored what the cleanup read under the key, null if it is absent
+ */
+ public void removeExpired(String key, String marker, byte[] stored) throws
PDException {
+ byte[] storeKey = getStoreKey(key);
+ byte[] markerKey = getStoreKey(marker);
+ if (stored == null) {
+ meta.remove(markerKey);
+ return;
+ }
+ V read;
+ try {
+ read = V.parseFrom(stored);
+ } catch (InvalidProtocolBufferException e) {
+ // Not a value KvService wrote: the plain delete of before
+ meta.remove(storeKey);
+ meta.remove(markerKey);
+ return;
+ }
+ int timeEnd = marker.indexOf(KV_DELIMITER, TTL_PREFIX.length() + 1);
+ long expireAt = Long.parseLong(marker.substring(TTL_PREFIX.length() +
1, timeEnd));
+ if (read.getTtl() != expireAt) {
+ // Written after the marker, with another TTL or none: not the
value it names
+ meta.remove(markerKey);
+ return;
+ }
+ if (!Arrays.equals(meta.getOne(storeKey), stored)) {
+ // The key changed since the read: keep it, drop the marker that
named the old value
+ meta.remove(markerKey);
+ return;
+ }
+ meta.remove(storeKey);
Review Comment:
Important: this deletes a value that has not expired when its marker sorts
inside the scan range by bytes but not by number.
`clearTTLData` selects markers with `scanRange(K@T@0@, K@T@<now>@)`, and the
RocksDB iterator compares bytes, so the time is compared as a decimal string.
Any `expireAt` with more digits than `now` whose leading digits compare lower
is selected. With `now` = 1792521611424, `putTTL(k, v, 10^13)` gives expireAt
11792521611424, and the marker `K@T@11792521611424@k` sorts below
`K@T@1792521611424@` (the second byte, `1` against `7`). The same holds for
TTLs of 9999999999999, 10^14, 99999999999999 and 10^15. For such a marker,
`read.getTtl() == expireAt` and the bytes still match, so this line removes a
value that `get` would still return as live. On master the misparse meant this
path only dropped the marker and left the value in place; with the parse fixed,
the value is deleted within a second of being written. `putTTL` does not
validate the TTL, so a client that passes a large number to mean "keep it"
loses the key.
Please delete the value only once it has actually expired, using the same
test `getValue` uses: keep the value when `read.getTtl() >=
System.currentTimeMillis()`, for example by checking `expireAt` against the
current time before the delete. Please also add a case to `KvServiceTest` with
`put(key, value, 10_000_000_000_000L)` followed by `clearTTLData()` that
asserts the value is still there.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
