bitflicker64 commented on code in PR #3382:
URL: https://github.com/apache/hugegraph/pull/3382#discussion_r4350969906
##########
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:
Fixed in 55ae8fee6: `clearTTLData` now skips an entry whose time is not a
run of decimal digits and goes on with the next one, so one ordinary key no
longer stops the sweep. Regression:
`testTtlCleanupSkipsAnOrdinaryKeyInTheMarkerRange` puts `T@123abc@foo` before
an expired marker, and checks that the ordinary key is kept and the expired key
deleted. It failed at 37ca25508 with the `NumberFormatException` you describe.
An ordinary key with a numeric time, such as `T@1700000000000@foo`, is still
taken for a marker, as on master, where every such key is deleted; that is
tracked in #3392.
##########
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:
Agreed: the compare and the remove have to be one raft entry, and that is
what the txn of #3376 adds, with an EQUALS compare on the value and the DELETE
of the value and its marker in the same entry. So once #3376 merges, I will
rebase #3382 onto it and move the cleanup to a single txn, with a test for a
renewal after the compare. Until then #3382 waits; no lock or new raft op here.
The PR body says so now.
--
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]
