xiangfu0 commented on PR #19270: URL: https://github.com/apache/pinot/pull/19270#issuecomment-5306305649
## Self-review pass (multi-domain) + fixes applied Ran an independent multi-domain review over the branch diff. **0 CRITICAL, 7 MAJOR.** Addressed the substantive findings in the latest commit: **Fixed** - **Schema re-migration infinite loop (verified bug):** marker-only schema migrations were persisted via `PinotHelixResourceManager.updateSchema`, which short-circuits on `schema.equals(oldSchema)` — and `Schema.equals()` deliberately excludes the migration marker, so the marker never persisted and the schema re-migrated every cycle (with false success metrics). Latent today (empty schema chain) but a real trap. Now persisted directly via `ZKMetadataProvider.setSchema` + best-effort refresh messages. Covered by a new test asserting the stored marker advances and a refresh is sent. - **Default flipped to OFF (opt-in):** the task rewrites stored ZK configs cluster-wide, so it now defaults off and is intended to be enabled only after all nodes are upgraded. Resolves the rolling-upgrade/rollback and soak-period concerns. - **Empty-chain short-circuit:** no ZK reads/writes when a migration chain is empty (the shipped schema chain is empty), and the schema is fetched once per table. - **Mixed-version field clearing:** documented on the migrator; verified current readers resolve via the `ingestionConfig`-preferred fallback, so same-version reads are unaffected — the opt-in-after-upgrade posture covers the old-binary-rollback edge. - **More tests:** deprecated ingestion-config collision folding (existing `ingestionConfig` wins, deprecated cleared), schema-marker persistence + refresh + once-per-hybrid-run, empty-chain short-circuit, and `Schema` serialization asserted through the full `objectToString` path. - Renamed `MIGRATION_VERSION_KEY` → `CONFIG_MIGRATION_VERSION_KEY`; documented the controller-managed reserved custom-config key. **Deferred (tracked, low-risk)** - A `ControllerTest`-based end-to-end test through the real ZK write path (current controller tests mock the resource manager) — worth adding but the unit coverage now exercises the persistence/skip/validation/metric paths. - SPI callout: this adds a new `org.apache.pinot.spi.config.migration` package and a `Schema.configMigrationVersion` field (both additive, rollback-safe) — flagging for plugin/enterprise maintainers. All precommit checks (spotless, license, checkstyle: 0 violations) pass on the four touched modules; unit suites green (spi 29, segment-local 6, controller 6). -- 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]
