gortiz commented on PR #19529: URL: https://github.com/apache/pinot/pull/19529#issuecomment-6013105831
Thanks for working on this. I agree that we need to know which decoder a consuming segment really uses during a migration, because the table config only says what it should use. But I'd like to reduce the scope of the PR before it goes in. The three channels it adds have very different costs, and only one of them is needed for the migration use case. | Channel | What it exposes | Persisted? | |---|---|---| | `decoderClassName` in the consuming segments info API | FQCN | No, computed per request | | `custom.decoder.class` in `metadata.properties` | FQCN | Yes, in every committed segment | | Segment ZK metadata (side effect, see below) | FQCN | Yes, in every segment ZNode | | `CONSUMING_SEGMENT_DECODER` gauge | simple class name | As a Prometheus series | The ZK one is the part that worries me most, and I don't think it is intentional. The converter writes the decoder under the `custom.` prefix, so `SegmentMetadataImpl.getCustomMap()` picks it up. Then at commit time `PinotLLCRealtimeSegmentManager` calls `SegmentZKMetadataUtils.updateCommittingSegmentZKMetadata`, which does `customMap.putAll(segmentMetadata.getCustomMap())`. The pauseless path in `TablesResource` does the same. The result is that every committed realtime segment in every cluster gets an extra `decoder.class` entry in its ZNode, with no way to opt out. Clusters with hundreds of thousands of segments will notice that in ZK size and in what brokers and controllers read. The PR description and tests don't cover it. Persisting the class name also has a design problem. Pinot already relies on fully qualified class names too much: they leak implementation details to users and make it hard to move classes between packages. We should slowly move toward semantic ids, not add more places where an FQCN becomes durable data. Once the FQCN is stored in every segment and in ZK, a package rename leaves historical data that points to classes that no longer exist. The simple name in the metric label has a related issue: two plugins can ship a class with the same simple name, and their label values would collide. About the gauge, I share @mcvsubbu's concern. It adds one JMX MBean and one Prometheus series per consuming partition per server. At the scale you describe, that is tens of thousands of series that lose most of their value once the migration ends. My suggestion is to keep the PR to the runtime API, which is also what @mcvsubbu proposed: 1. Keep `decoderClassName` in `SegmentConsumerInfo` / `ConsumingSegmentInfo` and the `getValueDecoder()` SPI default. These are not persisted, so they can evolve if we introduce decoder ids later. 2. Remove the `custom.decoder.class` segment metadata and the `RealtimeSegmentConverter` overload. That also removes the ZK side effect. 3. Remove the `CONSUMING_SEGMENT_DECODER` gauge, the new `setValueOfTableGauge` overload and the JMX exporter rules. If you really need a fleet-wide view, a follow-up behind a config flag (off by default) is easier to discuss on its own. 4. Keep the old constructors of `SegmentConsumerInfo` and `ConsumingSegmentInfo` and make them delegate to the new ones with `null`. Changing the only public constructor breaks source and binary compatibility for anyone who builds these DTOs outside this repo. You will need `@JsonCreator` on the new constructor so Jackson doesn't see two property-based creators. With that scope I'm happy to approve. If you need durable per-segment tracking after the migration, I'd rather design it around a semantic decoder id in the SPI than around the class name. -- 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]
