da-daken commented on code in PR #1024:
URL: https://github.com/apache/flink-agents/pull/1024#discussion_r3821679993
##########
runtime/src/main/java/org/apache/flink/agents/runtime/actionstate/ActionStateUtil.java:
##########
@@ -62,6 +68,25 @@ public static List<String> parseKey(String key) {
return List.of(parts);
}
+ /**
+ * Returns {@code true} if the composite {@code stateKey}'s business key
should be retained in a
+ * subtask's in-memory cache under the given ownership filter. A {@code
null} filter retains
+ * every key (the default for in-memory and test backends). If the key
cannot be parsed, it is
+ * retained as a fail-safe: prefer keeping a valid key over dropping it on
a parse error.
+ */
+ public static boolean isKeyRetained(
+ @Nullable Predicate<String> ownershipFilter, String stateKey) {
+ if (ownershipFilter == null) {
+ return true;
+ }
+ try {
+ return ownershipFilter.test(parseKey(stateKey).get(0));
Review Comment:
Good catch! Since the string key recovered during rebuild carries no type
information, we can't reconstruct the correct key-group from it (e.g. both
`Long(1)` and `String("1")` toString to `"1"`, but hash to different
key-groups).
The fix is to store the key-group directly at write time, when the typed key
is
still available, so recovery can read it back without relying on the string
form.
There are two options for where to store the key-group:
| Dimension | A (embed in key) | C (store in ActionState value) |
|---|---|---|
| **Correctness** | Correct. Key-group computed once from typed key at write
time. | Correct. |
| **Format change** | Key format (4 → 5 segments) | Value format (new field)
|
| **Upgrade behavior** | Old 4-segment keys are deterministically dropped +
warn log. At most one re-execution within the retention window. | Old values
without keyGroup field must be dropped, or fall back to the buggy string-hash —
which preserves the original bug. |
| **Upgrade cost** | One-time: old keys evicted after retention window. |
Same one-time cost. |
| **Self-describing** | Yes. The key alone tells you which key-group it
belongs to. | No. Must read the value to determine ownership. |
| **Identity vs. payload** | Key-group is identity information (like
business key, seqNum), belongs in the key. | Key-group is stored as payload,
mixing identity metadata with business data. |
| **Perf (Fluss rebuild)** | Filter by key before deserializing value. |
Must deserialize all values to read keyGroup. |
Both A and C have the same upgrade cost — old records lack the key-group in
either location and must be dropped. The difference is semantic: key-group is
derived from the typed key at write time, just like the business key and
sequence number. It belongs in the key, not in the value payload.
**I prefer A** because it keeps identity information in the key where it
belongs, avoids mixing identity metadata with business payload, and in Fluss
avoids deserializing values for non-owned keys during rebuild.
--
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]
