feat: add etag conditional write support to dynamic config#19501
feat: add etag conditional write support to dynamic config#19501jtuglu1 wants to merge 2 commits into
Conversation
0cc109c to
f66e71f
Compare
827f54d to
e71dd8f
Compare
e71dd8f to
aa5eba1
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
Reviewed 21 of 21 changed files.
This is an automated review by Codex GPT-5.5
3559fa0 to
055964f
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Follow-up checked: the disabled-CAS compaction path now returns a precondition failure for conditional writes, so the original issue is handled.
Reviewed 23 of 23 changed files.
This is an automated review by Codex GPT-5.5
a18d77f to
04df8e1
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Follow-up checked: the disabled-CAS compaction path now returns a precondition failure for conditional writes, and ETagged GET responses now derive the body and ETag from the same byte snapshot. I found one remaining concurrency issue in conditional partial updates.
Reviewed 23 of 23 changed files.
This is an automated review by Codex GPT-5.5
04df8e1 to
3028183
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
I found one conditional-write gap: the unified datasource compaction POST/DELETE paths still ignore If-Match when they fall back to dynamic config writes.
Reviewed 23 of 23 changed files.
Findings that could not be attached inline:
- indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordCompactionResource.java:284 - [P1] Honor If-Match on unified datasource compaction writes. When compaction supervisors are disabled, the unified datasource POST and DELETE paths still mutate the same DruidCompactionConfig document but call the configManager overloads without passing DynamicConfigEtagHelper.getIfMatch(...). A client can read an ETag for the compaction document, another writer can change it, and then the stale client can POST or DELETE /druid/indexer/v1/compaction/config/datasources/... with If-Match; the header is ignored and the write succeeds instead of returning 412. Thread the If-Match value through these datasource update/delete calls, or explicitly reject unsupported conditional writes, so conditional requests do not silently degrade to last-writer-wins.
This is an automated review by Codex GPT-5.5
| return true; | ||
| } | ||
| final String trimmed = ifMatchHeader.trim(); | ||
| if ("*".equals(trimmed)) { |
There was a problem hiding this comment.
why does it need * for match? instead of not setting if match header.
There was a problem hiding this comment.
This behavior is in accordance with the ETag RFC. Not too relevant in common case, but still keeps things spec-correct.
| @Nullable | ||
| public static String compute(@Nullable byte[] bytes) | ||
| { | ||
| if (bytes == null) { |
There was a problem hiding this comment.
imho it'd nice to use non null value maybe __NULL__ to represent that, WDYT?
There was a problem hiding this comment.
I think null for a null byte payload is fine. Any other sort of identifier would be more confusing IMO.
fce2929 to
e9fd702
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 23 of 23 changed files.
Findings that could not be attached inline:
- indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordCompactionResource.java:284 - [P1] Honor If-Match on unified datasource compaction writes. When compaction supervisors are disabled, the unified datasource POST and DELETE fallback paths still mutate the same DruidCompactionConfig document through overloads that do not receive DynamicConfigEtagHelper.getIfMatch(...). A client can read the compaction ETag, race with another config write, then POST or DELETE /druid/indexer/v1/compaction/config/datasources/... with a stale If-Match header and still get a successful write instead of 412. Thread the If-Match value through both fallback calls, or explicitly reject conditional requests there. This appears unchanged from the prior review.
This is an automated review by Codex GPT-5.5
@kfaraz @cecemei @FrankChen021 how do you folks want to handle these internal updates? ETag with retries seems like the best option, although I'm not sure allowing possibility for mutations to be completely blocked is acceptable. |
e9fd702 to
133da6d
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 23 of 23 changed files.
This is an automated review by Codex GPT-5.5
Findings that could not be attached inline:
- indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordCompactionResource.java:289 - [P1] Honor If-Match on unified datasource compaction writes. The previous If-Match gap is still present for the unified datasource compaction POST/DELETE fallback paths. When compaction supervisors are disabled, these branches still call the overloads that do not receive DynamicConfigEtagHelper.getIfMatch(...), so a client can read the compaction ETag, race with another config write, then POST or DELETE /druid/indexer/v1/compaction/config/datasources/... with a stale If-Match header and still get a successful write instead of 412. Thread the If-Match value through both fallback calls, or explicitly reject conditional requests there.
Description
Currently, competing writes to Druid dynamic configs can race with each other. This introduces the concept of conditional writes with etags, allowing clients some concurrency control over writes to Druid's dynamic config.
Callers can optionally provide an E-Tag header
If-Match: <ETag>in dynamic config payloads which the server will match against and reject with a 412 code (preconditions failed) instead of potentially returning 200 success (racy overwrite). Clients can retrieve a valid E-Tag value from issuing GETs on the corresponding dynamic config endpoint (read-modify-write).Release note
Add E-Tag conditional write support to dynamic config APIs
This PR has: