Skip to content

feat: add etag conditional write support to dynamic config#19501

Open
jtuglu1 wants to merge 2 commits into
apache:masterfrom
jtuglu1:support-conditional-dynamic-config-writes-via-etags
Open

feat: add etag conditional write support to dynamic config#19501
jtuglu1 wants to merge 2 commits into
apache:masterfrom
jtuglu1:support-conditional-dynamic-config-writes-via-etags

Conversation

@jtuglu1

@jtuglu1 jtuglu1 commented May 22, 2026

Copy link
Copy Markdown
Contributor

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:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@jtuglu1 jtuglu1 force-pushed the support-conditional-dynamic-config-writes-via-etags branch from 0cc109c to f66e71f Compare May 22, 2026 02:38
@jtuglu1 jtuglu1 force-pushed the support-conditional-dynamic-config-writes-via-etags branch 3 times, most recently from 827f54d to e71dd8f Compare May 22, 2026 03:35
@jtuglu1 jtuglu1 requested review from gianm and maytasm May 22, 2026 03:38
@jtuglu1 jtuglu1 force-pushed the support-conditional-dynamic-config-writes-via-etags branch from e71dd8f to aa5eba1 Compare May 22, 2026 06:11

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread processing/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java Outdated
@jtuglu1 jtuglu1 force-pushed the support-conditional-dynamic-config-writes-via-etags branch 2 times, most recently from 3559fa0 to 055964f Compare May 26, 2026 18:20

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jtuglu1 jtuglu1 force-pushed the support-conditional-dynamic-config-writes-via-etags branch 2 times, most recently from a18d77f to 04df8e1 Compare May 28, 2026 02:35

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jtuglu1 jtuglu1 force-pushed the support-conditional-dynamic-config-writes-via-etags branch from 04df8e1 to 3028183 Compare May 28, 2026 18:04
@jtuglu1 jtuglu1 requested a review from FrankChen021 May 28, 2026 18:47

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it need * for match? instead of not setting if match header.

@jtuglu1 jtuglu1 Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is in accordance with the ETag RFC. Not too relevant in common case, but still keeps things spec-correct.

Comment thread processing/src/main/java/org/apache/druid/common/config/ConfigEtag.java Outdated
Comment thread processing/src/main/java/org/apache/druid/common/config/ConfigManager.java Outdated
Comment thread processing/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java Outdated
@Nullable
public static String compute(@Nullable byte[] bytes)
{
if (bytes == null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imho it'd nice to use non null value maybe __NULL__ to represent that, WDYT?

@jtuglu1 jtuglu1 Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think null for a null byte payload is fine. Any other sort of identifier would be more confusing IMO.

@jtuglu1 jtuglu1 force-pushed the support-conditional-dynamic-config-writes-via-etags branch from fce2929 to e9fd702 Compare June 3, 2026 04:12

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jtuglu1

jtuglu1 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

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.

@jtuglu1 jtuglu1 requested a review from kfaraz June 13, 2026 17:53
@jtuglu1 jtuglu1 force-pushed the support-conditional-dynamic-config-writes-via-etags branch from e9fd702 to 133da6d Compare June 18, 2026 22:42
@jtuglu1 jtuglu1 added this to the 38.0.0 milestone Jun 19, 2026

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants