awsomesud347 commented on PR #12130: URL: https://github.com/apache/seatunnel/pull/12130#issuecomment-5565397571
@nzw921rx These are fair questions, and you are right that the scan itself is untouched. Let me start with why the old code was shaped this way, because it explains the size of the gap. The service method `getJobsByStateJson(String state)` takes no page arguments at all, so it has no notion of a page. Pagination lives one layer up, in `PageBaseServlet`, which could only slice the array after the service had already built every row. The service was therefore always doing the full amount of work regardless of how few rows the caller asked for. You are correct that pagination cannot speed up the scan, and it does not. `matchingJobStates()` is shared by both paths and performs the same `values()` fetch, the same filter and the same sort either way. What changed is the per-row work that happens after the scan. For each job, `toJobInfoJson` performs two IMap point lookups, a `getOrDefault` on `IMAP_FINISHED_JOB_METRICS` and a `get` on `IMAP_FINISHED_JOB_VERTEX_INFO`, and then builds the row through `getJobInfoJson`, which calls `getJobMetrics` and allocates a fresh `ObjectMapper` in order to `readTree` the metrics JSON at `BaseService.java:684`. At ten thousand retained jobs with `rows=10`, the old path performed twenty thousand point lookups and ten thousand metric aggregations and then discarded everything except ten rows. The new path performs twenty and ten. So pagination does reduce IMap operations, just not the scan. That is also the answer to what was sacrificed, which is nothing. This is not a trade of one cost against another. The old path was building rows that were thrown away microseconds later, and the change simply stops building them. The output is identical in content, ordering and `total`. The one genuine cost is that `matchingJobStates()` now collects into a `List` before mapping, which the previous code did not do, so the unpaged path allocates one additional list. The evidence I find most convincing is not a timing measurement at all. The test `JobInfoServiceNullSafetyTest#shouldApplyPageBeforePerJobLookups` seeds twenty five finished jobs, requests the first ten, and then asserts that exactly ten metric lookups and ten DAG lookups occurred. Because that is a counted invariant rather than a duration, it is unaffected by confidence intervals, JIT warmup or garbage collection. On the specific steps, the measurement used the existing `seatunnel-benchmarks` JMH harness rather than anything new. `IMapJobGrowthBenchmarkWorkload` starts a single member mini cluster, runs a real fixture job to completion so that the stored values are genuine Zeta objects, and then seeds `initialStoredJobCount` additional finished jobs into `IMAP_FINISHED_JOB_STATE` and `IMAP_FINISHED_JOB_METRICS`. I parameterised that at one thousand and ten thousand. Two benchmarks then ran against that same seeded state, one calling the paged overload and one calling the unpaged method, in `AverageTime` mode reported in microseconds. I ran it under WSL because the harness cannot start on Windows, where the mini cluster path is written into a double quoted YAML scalar and the backslashes are read as escape sequences. There are two caveats on those numbers that you should weigh. The first is that the baseline benchmark calls the refactored `getJobsByStateJson(state)` on this branch rather than the version on dev, so the ratio compares the unpaged and paged paths within one branch rather than dev against this PR. The second is that `preloadStoragePressure` stores the same `JobState` instance under every key, which means `jobState.getJobId()` returns the same identifier for all seeded rows and the metrics lookup therefore hits a single key repeatedly. Real traffic would spread across distinct keys and partitions, so the absolute figures are optimistic even though the ratio holds. On profiling, there is no flame graph or JFR yet. The only `-prof gc` run so far was against the counting benchmarks rather than these listing ones, so allocation data for this path is still missing. I am re-running now at the module's `BenchmarkBase` defaults with `-prof gc` added, which should show allocation falling in step with the lookup count, and I will post those figures. The wide confidence intervals were caused by my settings rather than by the workload. I ran with one fork, one warmup iteration and three measurement iterations in order to save time, which is far too few. The re-run uses three forks with three warmup and five measurement iterations. On #11494, thanks for the pointer. I have only read its description and file list so far rather than the diff, so please treat this as provisional. From those, it looks like it targets a different phase: it reduces memory during file-based IMap WAL recovery, which runs when state is rebuilt from disk, whereas the cost here is on the runtime read path serving a single HTTP request. Both follow from retained history growing, but I do not think one substitutes for the other. Do you see a connection I am missing? I am happy to read it properly and test locally if you think it bears on this. -- 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]
