Store xDS resources as YAML instead of JSON#1321
Merged
Merged
Conversation
Motivation: - xDS resources were stored as JSON files. YAML is more human-readable and is the preferred format for configuration in the Kubernetes/Envoy ecosystem. Modifications: - `XdsResourceManager`: `fileName()` now returns `.yaml`; `push()` uses `Change.ofYamlUpsert()`; `updateOrDelete()` falls back to the legacy `.json` path when the `.yaml` file is absent; `update()` atomically removes the old `.json` and creates the new `.yaml` in a single commit. - `XdsResourceWatchingService`: changed `handleXdsResource()` signature from `String contentAsText` to `JsonNode content`; both call sites now pass `entry.content()` / `change.content()` directly instead of `contentAsText()`. Also accepts `EntryType.YAML` and handles `UPSERT_YAML`. Result: - Newly created xDS resources are stored as `.yaml` files. - Existing `.json` files remain readable; they are atomically migrated to `.yaml` on the first update.
📝 WalkthroughWalkthroughXDS resource storage, watch handling, Kubernetes endpoint aggregation, webapp lookups, and tests were updated to use YAML resource paths with legacy JSON compatibility paths and migration handling. ChangesXDS YAML Migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant XdsResourceManager
participant XdsResourceWatchingService
participant XdsEndpointUpdateScheduler
participant Repository
XdsResourceManager->>Repository: find existing YAML or JSON
Repository-->>XdsResourceManager: resolved file
XdsResourceWatchingService->>Repository: receive YAML/JSON diff
XdsResourceWatchingService->>XdsEndpointUpdateScheduler: dispatch flush/update
alt legacy JSON migration
XdsEndpointUpdateScheduler->>Repository: push remove JSON + upsert YAML
else direct YAML update
XdsEndpointUpdateScheduler->>Repository: transform YAML content
end
Repository-->>XdsEndpointUpdateScheduler: commit result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java (1)
180-198: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the YAML-vs-JSON choice deterministic.
FIND_ONE_WITHOUT_CONTENToverbase.*can return the legacy.jsoneven when the.yamlcounterpart also exists, so this path may transform stale JSON and upsert it over the YAML content. Fetch both matches without content and prefer.yaml; only migrate.jsonwhen no YAML entry exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java` around lines 180 - 198, The selection logic in XdsEndpointUpdateScheduler.handle currently relies on FIND_ONE_WITHOUT_CONTENT over baseFileName + ".*", which can nondeterministically pick the legacy .json entry even when a .yaml counterpart exists. Update the lookup to fetch both matching entries without content, then explicitly prefer the .yaml path in the branch that calls executeTransform; only fall back to executeTransformWithMigration when no .yaml entry is present and the matched file is .json. Ensure the decision is based on the resolved entry name, not the arbitrary first result from the map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java`:
- Around line 248-249: The migration path in XdsEndpointUpdateScheduler
currently lets a failure from BatchUpdateTransformer.apply() abort the returned
stage without closing the copied gRPC observers. Update the callback around
entry.revision() / entry.content() so any exception from apply is caught and all
pending observers are completed or failed before propagating the error, similar
to how Command.transform handles cleanup.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java`:
- Around line 201-208: The resource lookup in XdsResourceManager is too broad
because baseFileName + ".*" can match unrelated sibling files and cause false
conflicts or wrong update/delete targets. Update the lookup in the
create/update/delete flow around the repository.find call to check only the
exact .yaml resource path and the legacy .json path, using the existing
fileName/baseFileName handling in XdsResourceManager so FIND_ONE_WITHOUT_CONTENT
only considers those two resource candidates.
- Around line 287-288: Retry the push when a legacy-file removal races with
migration: XdsResourceManager.updateOrDelete() can resolve a .json path that is
deleted and replaced by .yaml before executePush() runs, causing
Change.ofRemoval(legacyFileToRemove) to fail. Update the push flow around
executePush(), createLegacyChange, and the legacyFileToRemove handling so it
re-resolves the current resource path and retries with the YAML target instead
of surfacing the stale removal error.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java`:
- Around line 201-213: In XdsKubernetesEndpointFetchingService’s endpoint
cleanup flow, the current find/delete path only removes the first matching
endpoint file, so a leftover .json or .yaml counterpart can remain. Update the
repository.find(...).handle(...) logic to collect all matching endpoint paths
returned for the endpointBase prefix instead of using FIND_ONE_WITHOUT_CONTENT
and findFirst(), then call removeEndpointFile for each matched YAML/JSON path so
both legacy formats are cleaned up together.
- Around line 188-193: When an aggregator file is removed, the code in
XdsKubernetesEndpointFetchingService currently closes the corresponding
KubernetesEndpointsUpdater but leaves it in kubernetesEndpointsUpdaters. Update
the removal path to also delete the entry from the map after calling
updater.close(), using the existing aggregatorName key so stale updaters do not
accumulate.
---
Outside diff comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java`:
- Around line 180-198: The selection logic in XdsEndpointUpdateScheduler.handle
currently relies on FIND_ONE_WITHOUT_CONTENT over baseFileName + ".*", which can
nondeterministically pick the legacy .json entry even when a .yaml counterpart
exists. Update the lookup to fetch both matching entries without content, then
explicitly prefer the .yaml path in the branch that calls executeTransform; only
fall back to executeTransformWithMigration when no .yaml entry is present and
the matched file is .json. Ensure the decision is based on the resolved entry
name, not the arbitrary first result from the map.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc455d39-0640-4f89-8c2e-49525ea44751
📒 Files selected for processing (25)
it/xds-k8s-node-ip-extractor/src/test/java/com/linecorp/centraldogma/it/xds/k8s/XdsKubernetesNodeIpExtractorTest.javaserver/src/main/java/com/linecorp/centraldogma/server/command/ContentTransformer.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/AbstractChangesApplier.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/TransformingChangesApplier.javawebapp/src/dogma/features/xds/K8sAggregatorStatus.tsxwebapp/src/dogma/features/xds/XdsTypes.tswebapp/src/dogma/features/xds/xdsApiSlice.tsxds/src/main/java/com/linecorp/centraldogma/xds/cluster/v1/XdsClusterService.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointService.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/CentralDogmaXdsResources.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlaneService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingService.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.javaxds/src/main/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerService.javaxds/src/main/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteService.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadPermissionTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsLegacyJsonCompatibilityTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsWritePermissionTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/AggregatingMultipleKubernetesTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java (1)
199-254: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftLegacy
.json→.yamlmigration path lacks the atomicity guarantee of theCommand.transformpath.The
.yamlbranch (lines 233-254) usesCommand.transformwith aContentTransformer, which recomputes against the latest content at commit time. The legacy.jsonmigration branch (lines 204-232) instead reads content once viarepository.get(Revision.HEAD, resolvedFileName), computesnewJsonNodesynchronously, and pushes a staticChange.ofRemoval+Change.ofYamlUpsertpair — a raw overwrite with no "expected old value" check.Given
BatchUpdateTask.flush()releases its lock before the async I/O (pendingUpdates.clear()/scheduledFuture = nullhappen beforerepository.find(...)/.get(...)completes), a new update arriving for the same endpoint during that window can schedule an overlappingflush(). If both flushes race down the legacy-migration branch for the same file, the second push may be based on stale content, potentially silently dropping the first flush's register/deregister changes instead of surfacing a conflict.Please confirm whether
Command.pushperforms conflict detection for full-contentChange.ofYamlUpsert/Change.ofRemovalagainst a movingRevision.HEAD, or add an explicit guard (e.g., patch-style expected-old-value check, similar toChange.ofJsonPatch(fileName, null, jsonNode)used for creates inXdsResourceManager) so the migration path detects concurrent changes rather than silently overwriting them.#!/bin/bash # Inspect how full-content upsert/removal changes are conflict-checked during commit application. rg -n -B5 -A15 'ChangeConflictException' --type=java xds server | rg -n -B15 'ofYamlUpsert|ofJsonUpsert|ofRemoval'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java` around lines 199 - 254, The legacy .json-to-.yaml migration branch in XdsEndpointUpdateScheduler uses a read-then-push flow that can race with overlapping flushes and overwrite newer changes. Update the migration path so Command.push is either confirmed to enforce conflict detection for Change.ofRemoval and Change.ofYamlUpsert against Revision.HEAD, or add an explicit expected-old-value/patch-style guard like the create path uses in XdsResourceManager. Keep the fix localized around the resolvedFileName branch and the BatchUpdateTransformer-driven commit flow so concurrent updates fail with a conflict instead of silently replacing content.
🧹 Nitpick comments (1)
xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java (1)
85-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate of
XdsResourceManager.alternativeFileName.Identical logic already exists as a private static method in
XdsResourceManager(seexds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java, lines 85-93). Consider exposing it as a package-private/public static utility there (or extracting to a shared helper class) and reusing it here instead of re-implementing the same extension-swap logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java` around lines 85 - 94, The alternativeFileName logic in XdsEndpointUpdateScheduler duplicates the existing XdsResourceManager alternativeFileName implementation; remove the local copy and reuse the shared method instead. Either make XdsResourceManager.alternativeFileName accessible from this package or extract the extension-swap helper into a shared utility, then update XdsEndpointUpdateScheduler to call that shared symbol rather than reimplementing the .json/.yaml conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java`:
- Around line 199-254: The legacy .json-to-.yaml migration branch in
XdsEndpointUpdateScheduler uses a read-then-push flow that can race with
overlapping flushes and overwrite newer changes. Update the migration path so
Command.push is either confirmed to enforce conflict detection for
Change.ofRemoval and Change.ofYamlUpsert against Revision.HEAD, or add an
explicit expected-old-value/patch-style guard like the create path uses in
XdsResourceManager. Keep the fix localized around the resolvedFileName branch
and the BatchUpdateTransformer-driven commit flow so concurrent updates fail
with a conflict instead of silently replacing content.
---
Nitpick comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java`:
- Around line 85-94: The alternativeFileName logic in XdsEndpointUpdateScheduler
duplicates the existing XdsResourceManager alternativeFileName implementation;
remove the local copy and reuse the shared method instead. Either make
XdsResourceManager.alternativeFileName accessible from this package or extract
the extension-swap helper into a shared utility, then update
XdsEndpointUpdateScheduler to call that shared symbol rather than reimplementing
the .json/.yaml conversion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02eb782e-57e2-48df-b1b2-16a41823e562
📒 Files selected for processing (10)
xds/src/main/java/com/linecorp/centraldogma/xds/cluster/v1/XdsClusterService.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointService.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.javaxds/src/main/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerService.javaxds/src/main/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteService.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsRegisterEndpointTest.java (1)
259-309: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider asserting migrated content correctness.
The test verifies
.yamlpresence and.jsonabsence post-migration but doesn't verify that the migrated.yamlfile actually contains the correct pre-migration data (e.g., viacheckEndpointsViaDiscoveryRequestor content assertion). Adding this would guard against silent data-loss regressions during migration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsRegisterEndpointTest.java` around lines 259 - 309, The migration test currently only checks that the legacy .json file disappears and the .yaml file exists, but it does not verify that the migrated content matches the original endpoint data. In registerAndDeregisterMigratesLegacyJsonFile, add an assertion using the existing discovery/content verification helpers such as checkEndpointsViaDiscoveryRequest or an equivalent repository content check to confirm the .yaml produced from the legacy JSON preserves the pre-migration ClusterLoadAssignment and endpoints.xds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.java (1)
379-417: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider asserting migrated endpoint content correctness.
Similar to the endpoint-registration migration test, this only checks presence/absence of
.yaml/.jsonfiles, not that the migrated content matcheslegacyEndpoints. A content-equality assertion (or a discovery-request check) would strengthen coverage of the migration path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.java` around lines 379 - 417, The migration test in XdsKubernetesServiceTest#createAggregator_migratesLegacyJsonEndpoint only verifies that the .yaml file exists and the old .json file is removed, but it does not confirm the migrated content is preserved. Update the test to also assert that the migrated endpoint entry from fooGroup.getOrNull(Query.ofYaml(K8S_ENDPOINTS_DIRECTORY + aggregatorId + ".yaml")) matches the original legacyEndpoints (or equivalent discovery output), so the migration path is validated end-to-end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java`:
- Around line 351-392: The ChangeConflictException fallback in
XdsKubernetesEndpointFetchingService’s migration handler is too broad and can
hide real YAML upsert conflicts. Update the Command.push(...).handle(...) logic
so legacyJsonMigrated is set to true only when the conflict specifically means
the legacy .json file is missing, and otherwise keep the removal+upsert cleanup
path and let the error surface; use the existing peeled exception handling
around RedundantChangeException and ChangeConflictException to narrow the
fallback.
---
Nitpick comments:
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsRegisterEndpointTest.java`:
- Around line 259-309: The migration test currently only checks that the legacy
.json file disappears and the .yaml file exists, but it does not verify that the
migrated content matches the original endpoint data. In
registerAndDeregisterMigratesLegacyJsonFile, add an assertion using the existing
discovery/content verification helpers such as checkEndpointsViaDiscoveryRequest
or an equivalent repository content check to confirm the .yaml produced from the
legacy JSON preserves the pre-migration ClusterLoadAssignment and endpoints.
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.java`:
- Around line 379-417: The migration test in
XdsKubernetesServiceTest#createAggregator_migratesLegacyJsonEndpoint only
verifies that the .yaml file exists and the old .json file is removed, but it
does not confirm the migrated content is preserved. Update the test to also
assert that the migrated endpoint entry from
fooGroup.getOrNull(Query.ofYaml(K8S_ENDPOINTS_DIRECTORY + aggregatorId +
".yaml")) matches the original legacyEndpoints (or equivalent discovery output),
so the migration path is validated end-to-end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e5f06ef-e9f1-47f4-9f7f-7d972ca40e02
📒 Files selected for processing (22)
it/xds-k8s-node-ip-extractor/src/test/java/com/linecorp/centraldogma/it/xds/k8s/XdsKubernetesNodeIpExtractorTest.javawebapp/src/dogma/features/xds/K8sAggregatorStatus.tsxwebapp/src/dogma/features/xds/XdsTypes.tswebapp/src/dogma/features/xds/xdsApiSlice.tsxds/src/main/java/com/linecorp/centraldogma/xds/cluster/v1/XdsClusterService.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointService.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingService.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.javaxds/src/main/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerService.javaxds/src/main/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteService.javaxds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsRegisterEndpointTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadPermissionTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsLegacyJsonCompatibilityTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsWritePermissionTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/AggregatingMultipleKubernetesTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.java
✅ Files skipped from review due to trivial changes (2)
- xds/src/main/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteService.java
🚧 Files skipped from review as they are similar to previous changes (13)
- webapp/src/dogma/features/xds/XdsTypes.ts
- xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsWritePermissionTest.java
- xds/src/main/java/com/linecorp/centraldogma/xds/cluster/v1/XdsClusterService.java
- xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadPermissionTest.java
- xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingServiceTest.java
- xds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/AggregatingMultipleKubernetesTest.java
- it/xds-k8s-node-ip-extractor/src/test/java/com/linecorp/centraldogma/it/xds/k8s/XdsKubernetesNodeIpExtractorTest.java
- xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadServiceTest.java
- xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java
- xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsLegacyJsonCompatibilityTest.java
- webapp/src/dogma/features/xds/xdsApiSlice.ts
- xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java
- xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingService.java
jrhee17
approved these changes
Jul 8, 2026
jrhee17
left a comment
jrhee17
left a comment
Contributor
There was a problem hiding this comment.
Question: It seems like migration is done lazily (on each push for a file)
Will there be any attempt to migrate at once? Or would it be helpful if we can at least visualize the number of json files vs yaml files that are used in the xds repo?
| } | ||
| // If the change is redundant, we just ignore it and complete | ||
| // the stream observer without error. | ||
| if (resolvedFileName.endsWith(".json")) { |
Contributor
There was a problem hiding this comment.
Note) Understood the assumption is that only one of yaml or json had been stored
| case REMOVE: | ||
| // Skip if this is an atomic .json → .yaml migration commit: the .json is being | ||
| // removed while the same resource is upserted as .yaml in the same diff. | ||
| if (path.endsWith(".json")) { |
Contributor
Author
Yeah, I'm preparing. 😉
I have a command for checking it. 😉 |
This was referenced Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
Modifications:
XdsResourceManager:fileName()now returns.yaml;push()usesChange.ofYamlUpsert();updateOrDelete()falls back to the legacy.jsonpath when the.yamlfile is absent;update()atomically removes the old.jsonand creates the new.yamlin a single commit.XdsResourceWatchingService: changedhandleXdsResource()signature fromString contentAsTexttoJsonNode content; both call sites now passentry.content()/change.content()directly instead ofcontentAsText(). Also acceptsEntryType.YAMLand handlesUPSERT_YAML.Result:
.yamlfiles..jsonfiles remain readable; they are atomically migrated to.yamlon the first update.