imbajin commented on code in PR #3382:
URL: https://github.com/apache/hugegraph/pull/3382#discussion_r4341950595
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/KvService.java:
##########
@@ -297,18 +298,70 @@ 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. A marker whose time has not passed is left
alone with its value,
+ * which a read still returns. 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 {
+ int timeEnd = marker.indexOf(KV_DELIMITER, TTL_PREFIX.length() + 1);
+ long expireAt = Long.parseLong(marker.substring(TTL_PREFIX.length() +
1, timeEnd));
Review Comment:
⚠️ Important: An ordinary KV key such as `T@123abc@foo` is stored as
`K@T@123abc@foo`, inside the scanned range before today's numeric TTL markers.
`Long.parseLong` throws for its non-numeric time field, and because
`clearTTLData` catches outside the whole loop this entry aborts the sweep and
leaves later expired markers behind on every run. Please validate candidates
per entry and continue after a malformed key, or reserve and validate the
marker namespace; add a regression with this malformed key before a valid
expired marker.
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/KvService.java:
##########
@@ -297,18 +298,70 @@ 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. A marker whose time has not passed is left
alone with its value,
+ * which a read still returns. 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 {
+ int timeEnd = marker.indexOf(KV_DELIMITER, TTL_PREFIX.length() + 1);
+ long expireAt = Long.parseLong(marker.substring(TTL_PREFIX.length() +
1, timeEnd));
+ if (expireAt >= System.currentTimeMillis()) {
+ // The scan compares bytes, so a longer time can sort below now:
not due yet
+ return;
+ }
+ 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;
+ }
+ 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:
‼️ Critical: There is still a time-of-check/time-of-use window after the
equality check. `meta.getOne(storeKey)` can return the inspected bytes, then a
concurrent `put` or `keepAlive` can renew the value before this separate
`meta.remove(storeKey)` Raft operation; cleanup will delete the renewed value
and leave its new TTL marker behind. Please perform the compare-and-delete and
marker removal atomically in one Raft entry, and cover a renewal after the
equality check.
--
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]
