SEZ9 commented on PR #12165: URL: https://github.com/apache/seatunnel/pull/12165#issuecomment-6258917219
Thanks @Vivek1106-04 for picking up the benchmark question first, and thanks @nzw921rx for the trace and the CV observation. Where things stand from my side: - The head of this PR is still `c773cc091`; nothing new has been pushed since the earlier review, so the source-side conclusion on that commit is unchanged and the CI attribution on fork run `35729456152` still applies to it. - The CV-shrinking-with-pipeline-count behaviour @nzw921rx observed is a property of the measurement harness in https://github.com/apache/seatunnel/pull/12418, not of the code in this PR, so I agree with the sequencing already agreed in this thread: fix the harness, then re-run this PR against `dev`. - I have read the analysis and proposed fix in https://github.com/apache/seatunnel/pull/12418#issuecomment-6135494803. I have not re-run the sweep myself, so I am treating those numbers as the author's measurements rather than something I verified. Concrete remaining asks: 1. @Vivek1106-04 — please open the harness fix as a PR (the comment in #12418 mentions an open question before doing so; if that is still unresolved, please state the question here so it does not stall). Link it on this thread once it exists. 2. After the harness fix lands, re-run the comparison of this PR against `dev` and post the result here, not only on #12418, so the efficiency evidence lives with the change it is meant to justify. 3. If the rerun requires any commit on top of `c773cc091`, or if you sync with `dev` first, please say so explicitly in the comment that posts the results, so the CI run can be redone on the new head rather than carried over from the earlier fork run. 4. @nzw921rx — once the rerun is posted, could you confirm whether the CV trend now looks sane to you, since you are the one who surfaced it? Final merge still needs a committer's look and a green CI run on whichever head ends up being the final one; I will re-review promptly if the head changes. <!-- streview-comment:1870 --> -- 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]
