You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "anuragrai16 (via GitHub)" <gi...@apache.org> on 2026/09/11 11:37:33 UTC
[PR] [pinot-server] Add consuming-segment decoder observability [pinot]
anuragrai16 opened a new pull request, #19529:
URL: https://github.com/apache/pinot/pull/19529
Expose the concrete `StreamMessageDecoder` class of a consuming realtime segment for debugging decode issues:
- New `CONSUMING_SEGMENT_DECODER` server gauge, tagged by table/topic/ partition/decoder class, emitted alongside `LLC_PARTITION_CONSUMING` and removed on segment offload.
- Persist the decoder class in segment metadata (custom.decoder.class) and surface decoderClassName via the consuming-segment debug APIs.
- Add a JMX->Prometheus scrape rule for the new gauge.
**Backward compatible:** SPI addition is a nullable default method, the DTO field is nullable with ignore-unknown, segment metadata is additive, and the scrape rule is anchored to avoid shadowing existing rules.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [pinot-server] Add consuming-segment decoder observability [pinot]
Posted by "mcvsubbu (via GitHub)" <gi...@apache.org>.
mcvsubbu commented on PR #19529:
URL: https://github.com/apache/pinot/pull/19529#issuecomment-5635558377
If I understand right, you are emitting a metric (of value 1) with the current decoder class name. Is this not an overkill? Can you just log the decoder class name? Even log at some regular cadence if needed?
I saw a proposal float by for pinot internal table. Perhaps you could add a row/column in that table?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [pinot-server] Add consuming-segment decoder observability [pinot]
Posted by "anuragrai16 (via GitHub)" <gi...@apache.org>.
anuragrai16 commented on PR #19529:
URL: https://github.com/apache/pinot/pull/19529#issuecomment-6464806831
Thanks @gortiz , agree with the scope reduction for now. Updated the PR to keep only the runtime API. Agreed on not persisting the FQCN since it has its own problem. If we need durable per-segment tracking later, I'll think of how to design it around a decoder id. I'll also treat any fleet-wide metric as a separate follow-up, behind a flag that's off by default.
- **Removed**: the gauge, the setValueOfTableGauge overload, the JMX rules, and the custom.decoder.class metadata along with the converter overload. That also takes out the ZK side effect you found, which wasn't intentional.
- **Kept**: decoderClassName on the DTOs and the getValueDecoder() SPI default. The value is computed per request and nothing is persisted.
- **Compatibility**: the old DTO constructors are back and delegate with null, and the new ones carry @JsonCreator.
There are tests for the legacy constructors, JSON round-tripping, and payloads from older servers. Javadoc is switched to the markdown style.
Can you do a re-review please ?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [pinot-server] Add consuming-segment decoder observability [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [pinot-server] Add consuming-segment decoder observability [pinot]
Posted by "anuragrai16 (via GitHub)" <gi...@apache.org>.
anuragrai16 commented on PR #19529:
URL: https://github.com/apache/pinot/pull/19529#issuecomment-5644479699
> If I understand right, you are emitting a metric (of value 1) with the current decoder class name. Is this not an overkill? Can you just log the decoder class name? Even log at some regular cadence if needed?
>
> I saw a proposal float by for pinot internal table. Perhaps you could add a row/column in that table?
Hi @mcvsubbu - Thanks for checking the diff. We're trying to migrate 1000s of table decoders on live production tables, each of which consumes from Kafka topics upto 256 partitions, while Pinot has no observability into what decoder was used for a mutable segment, or being used for a consuming segment. A log is ephemeral and not a reliable way to solve this problem.
At the cost/overkill part, we are just emitting one additional gauge per consuming segment, using the existing setValueOfGauge register-once path, no new gauge objects created per tick, just an AtomicLong.set(1). The overhead is negligible compared to what each consuming segment already emits.
As for the proposal for Pinot internal table, it is a larger effort that's still in design. This gauge is a minimal, backward-compatible addition (constant 1 emitted alongside the already-existing `LLC_PARTITION_CONSUMING` gauge on the same code path, same register-once/AtomicLong mechanism) that solves the immediate operational need without waiting on new infrastructure. Let me know your thoughts, I feel this metadata belongs in the segment as a first class citizen.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [pinot-server] Add consuming-segment decoder observability [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #19529:
URL: https://github.com/apache/pinot/pull/19529#issuecomment-6013202005
BTW, a nit: AFAIK, in javadoc using markdown style, the old `{@link ...}` is supported, but it is recommended to use the markdown style [text](link)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [pinot-server] Add consuming-segment decoder observability [pinot]
Posted by "mcvsubbu (via GitHub)" <gi...@apache.org>.
mcvsubbu commented on PR #19529:
URL: https://github.com/apache/pinot/pull/19529#issuecomment-5649678044
> > If I understand right, you are emitting a metric (of value 1) with the current decoder class name. Is this not an overkill? Can you just log the decoder class name? Even log at some regular cadence if needed?
> > I saw a proposal float by for pinot internal table. Perhaps you could add a row/column in that table?
>
> Hi @mcvsubbu - Thanks for checking the diff. We're trying to migrate 1000s of table decoders on live production tables, each of which consumes from Kafka topics upto 256 partitions, while Pinot has no observability into what decoder was used for a mutable segment, or being used for a consuming segment. A log is ephemeral and not a reliable way to solve this problem.
>
> At the cost/overkill part, we are just emitting one additional gauge per consuming segment, using the existing setValueOfGauge register-once path, no new gauge objects created per tick, just an AtomicLong.set(1). The overhead is negligible compared to what each consuming segment already emits.
>
> As for the proposal for Pinot internal table, it is a larger effort that's still in design. This gauge is a minimal, backward-compatible addition (constant 1 emitted alongside the already-existing `LLC_PARTITION_CONSUMING` gauge on the same code path, same register-once/AtomicLong mechanism) that solves the immediate operational need without waiting on new infrastructure. Let me know your thoughts, I feel this metadata belongs in the segment as a first class citizen.
Every metric you add is an overhead. Especially ones that emit a 0 or 1. We can probably remove the consuming metric as well, because we now have a metric that shows the delay in consumption.
Moreover, the metric will be useless after migration (perhaps), when things are in stable state.
That being said, here are two alternatives:
- Get the information from the table config in zookeeper.
- Create an API on the server to get this information
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [pinot-server] Add consuming-segment decoder observability [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19529:
URL: https://github.com/apache/pinot/pull/19529#issuecomment-5634334709
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19529?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 100.00%. Comparing base ([`63b3b1f`](https://app.codecov.io/gh/apache/pinot/commit/63b3b1f1b3c8e46d38b46dfbe0d3fb8e29f6da43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`deba872`](https://app.codecov.io/gh/apache/pinot/commit/deba87205e8fd7a2ff40bd5586d2bb0ea021ba6a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19529 +/- ##
==============================================
+ Coverage 67.70% 100.00% +32.29%
+ Complexity 1450 6 -1444
==============================================
Files 3490 3 -3487
Lines 224998 6 -224992
Branches 35523 0 -35523
==============================================
- Hits 152335 6 -152329
+ Misses 60618 0 -60618
+ Partials 12045 0 -12045
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+32.29%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+32.29%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19529/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19529?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org