You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2026/08/26 14:15:57 UTC
[PR] Add per-role continuous JFR recording to the Helm chart [pinot]
gortiz opened a new pull request, #19372:
URL: https://github.com/apache/pinot/pull/19372
## What
Adds continuous [Java Flight Recorder](https://docs.oracle.com/en/java/javase/21/jfapi/) recording
to the Helm chart, per role, and deprecates the `pinot.jfr.*` cluster configs that did the same job
from inside the JVM.
The point is post-mortem profiling: when a server falls over, the profile of the minutes leading up
to it should already be on disk rather than something you go and enable afterwards.
```yaml
server:
jfr:
enabled: true
jfr: # shared by every role that enables it
configuration: default # ~1% overhead
maxSize: 2Gi
persistence:
enabled: false # true keeps recordings across pod rescheduling
```
That renders into the role's `JAVA_OPTS`:
```
-XX:FlightRecorderOptions=repository=/var/pinot/jfr,preserve-repository=false,maxchunksize=12582912
-XX:StartFlightRecording=name=pinot,settings=default,disk=true,maxsize=2147483648,dumponexit=false
```
## Why JVM arguments rather than the existing `pinot.jfr.*` cluster configs
`ContinuousJfrStarter` starts the recording after the component has connected to Helix. Starting it
from the command line is better in three ways:
- **Coverage.** The listener cannot run before cluster config has been read, so class loading,
plugin init, segment preload and the ZooKeeper connect itself are never captured.
- **Availability.** It needs ZooKeeper reachable, which is not a safe assumption during the
incidents a profile would help with.
- **Data loss.** `JFR.stop` without a filename deletes the whole repository, so changing any
`pinot.jfr.*` key — including one that only affects cleanup — silently discards every chunk
recorded so far.
Nothing is removed. The configs are `@Deprecated(forRemoval = true)`, still work, and warn once
naming the JVM arguments to use instead.
## Why there is an init container
JFR's `maxsize` makes the recording roll like a log file, so nothing needs to rotate files by hand.
What JFR does *not* do is reclaim the repository of a JVM that has already exited — and
`preserve-repository=true`, which is what keeps recordings across a restart in the first place,
means those directories survive on a PersistentVolume forever. Measured: a second run against the
same repository root leaves the first run's directory untouched, so each restart leaks up to a full
`maxSize`.
`jfr-janitor` reclaims them. Running it as an init container is what makes it safe — init containers
finish before the Pinot container starts, so every repository it sees belongs to a run that is
already over. It also refuses to delete anything written to within `jfr.janitor.minIdleMinutes`, and
always exits 0: a failed cleanup must never keep a role from starting.
## Design notes
- **Its own volume.** Never a subdirectory of the role's data volume, so a runaway recording cannot
eat the space Pinot needs for segments.
- **`jfr.persistence.enabled` defaults to false.** An `emptyDir` applies with a plain rolling
restart; a PersistentVolume adds a `volumeClaimTemplates` entry, which Kubernetes forbids changing
in place. `UPGRADING.md` has the one-time `--cascade=orphan` procedure.
- **The stateless minion always uses an `emptyDir`.** It is a Deployment, so replicas cannot each
have their own volume, and sharing one would let a starting pod's cleanup delete a running pod's
live recording during a rolling update.
- **One unit convention.** Every size is a Kubernetes quantity; the chart converts to the byte counts
JFR accepts. JFR's own unit table never reaches `values.yaml`, and the cleanup script parses no
units at all.
- **Values validated at render time.** A bad JFR option is not a warning — the JVM refuses to start —
so the chart fails `helm install` with a clear message instead of leaving a CrashLoopBackOff. That
includes checking the volume can hold the janitor's budget plus the runs that follow it.
## Getting a recording out
```bash
# snapshot a running JVM, without interrupting the recording
kubectl exec <pod> -- jcmd 1 JFR.dump name=pinot filename=/tmp/snap.jfr
# after a crash: rebuild from the repository left behind, including the chunk
# that was still open when the JVM died
kubectl exec <pod> -- jfr assemble /var/pinot/jfr/<repository-dir> /tmp/crash.jfr
```
Each `*.jfr` chunk in the repository is a valid recording on its own and is named with its start
timestamp, so you can pull only the window you care about instead of the whole volume.
## Testing
- `helm lint --strict`, `helm template` and `kubeconform -strict` over six configurations: defaults,
all five roles on with `emptyDir`, all five with a PVC, janitor off, a user-supplied
`initContainers` entry alongside the janitor, and a large `profile` setup.
- With JFR disabled the rendered manifests are identical to master except that an empty
`initContainers: []` key is no longer emitted.
- Negative cases assert `helm template` *fails*: `maxSize: 500m` (a lowercase `m` is *milli* in
Kubernetes), `maxSize: 2GB`, `maxAge: P7D`, a janitor with neither bound set, and an
over-committed volume.
- `helm/pinot/scripts/jfr-janitor-test.sh` — 13 fixture checks over the cleanup script: both passes,
non-repository entries left alone, a recently written repository never deleted, a malformed budget
skipping the pass rather than deleting everything, and exit 0 on a read-only or missing directory.
`shellcheck -s sh` clean.
- `ContinuousJfrStarterTest` — 29 cases, including a data provider over the JVM-argument detection
(both flags, both `=`/`:` forms, and `-Dsomething=-XX:StartFlightRecording` which must *not*
match).
- End to end on JDK 25: ran a JVM with the flags the chart renders, confirmed the repository layout,
recovered it with `jfr assemble`, and ran the janitor extracted from the rendered manifest against
the result.
## Notes for reviewers
- The chart has no CI job today (`helm lint`/`helm template` appear in none of the workflows). Happy
to add one in a follow-up if that is wanted — it would have caught two bugs found during review.
- `@Deprecated(forRemoval = true)` names no removal release. Suggestions welcome on which one to
target.
--
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] Add per-role continuous JFR recording to the Helm chart [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz closed pull request #19372: Add per-role continuous JFR recording to the Helm chart
URL: https://github.com/apache/pinot/pull/19372
--
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] Add per-role continuous JFR recording to the Helm chart [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #19372:
URL: https://github.com/apache/pinot/pull/19372#issuecomment-6042509358
Closing this one. I've been working on a different approach that I like more, and I'll open a new PR with it soon.
--
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] Add per-role continuous JFR recording to the Helm chart [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19372:
URL: https://github.com/apache/pinot/pull/19372#issuecomment-5427317346
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19372?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `89.47368%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.57%. Comparing base ([`d40dceb`](https://app.codecov.io/gh/apache/pinot/commit/d40dcebf1b0a9dd689403be0df771c069cd20059?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`afd9e81`](https://app.codecov.io/gh/apache/pinot/commit/afd9e81683ffa395715f76ca61220857e1e4a0d4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19372?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...he/pinot/core/util/trace/ContinuousJfrStarter.java](https://app.codecov.io/gh/apache/pinot/pull/19372?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Futil%2Ftrace%2FContinuousJfrStarter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL3RyYWNlL0NvbnRpbnVvdXNKZnJTdGFydGVyLmphdmE=) | 89.47% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19372?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (d40dceb) and HEAD (afd9e81). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (d40dceb) | HEAD (afd9e81) |
>|------|------|------|
>|lane-a|2|0|
>|integration|4|0|
>|java-25|6|1|
>|integration1|2|0|
>|temurin|6|1|
>|lane-b|2|0|
>|unittests|2|1|
>|integration2|2|0|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19372 +/- ##
============================================
- Coverage 67.44% 57.57% -9.87%
+ Complexity 1430 1 -1429
============================================
Files 3485 2682 -803
Lines 223874 163729 -60145
Branches 35300 26552 -8748
============================================
- Hits 150987 94274 -56713
- Misses 60890 61494 +604
+ Partials 11997 7961 -4036
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19372/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/19372/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19372/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19372/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/19372/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.57% <89.47%> (-9.87%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19372/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19372/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19372/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.57% <89.47%> (-9.87%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19372/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.57% <89.47%> (-9.87%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19372/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.57% <89.47%> (+0.03%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19372/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/19372?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