arunkumarucet commented on PR #19434: URL: https://github.com/apache/pinot/pull/19434#issuecomment-5584889482
Thanks for the thorough review @KKcorps — all six inline findings are addressed in 55a40be (threaded replies on each with the covering test). On the review-level items: **Fallback control / operational visibility:** the fallback is now explicitly controllable at two levels: a dynamically updatable cluster config `pinot.server.protobuf.descriptor.fallback.enabled` (default `true`; propagated live via a `PinotClusterConfigChangeListener` registered in `BaseServerStarter`, mirroring `ConsumingSegmentConsistencyModeListener`), overridable per table by the decoder prop `descriptorFileFallbackEnabled`. The default-on behavior and its operational implications (stale-serving is possible only during an actual fetch outage, WARN-logged per occurrence, never after a successful fetch of changed content) are documented on the config constants and in the updated PR description. I kept the default on because the motivating incidents would not have been prevented by an opt-in flag nobody had set — but happy to flip the default if you feel opt-in is the right posture for OSS. **Double parse / failure labeling:** the decoder now resolves its descriptor through `ProtoBufUtils.getDescriptor` — single parse, shared validated result — and fetch failures vs. invalid-content failures produce distinct errors (fallback WARN vs. `Invalid protocol buffer descriptor set`). **Commit trailer:** removed via history rewrite; the branch was force-pushed (`783b7ac` → `a871404` + `55a40be`). -- 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]
