rzo1 commented on PR #9097: URL: https://github.com/apache/storm/pull/9097#issuecomment-6387518054
Thanks for addressing all the code-level points, the tests now exercise the real paths nicely. One open question from my earlier review: should the flag cover all seven pools, or only the Thrift handler pools? For the small pools (supervisor heartbeat, size 1; `AsyncLocalizer` tasks, size 3; `AssignmentDistributionService`), the saving is a handful of threads. On the other hand, those threads disappear from `Utils.threadDump()`, which is what Storm logs when a slot gets stuck. I would prefer limiting the flag to the Thrift pools, or alternatively making the logged dump include virtual threads. Minor: - `flight.bash` writes the jcmd output as `threaddump-<pid>-<ts>.log`. The log cleaner treats `.log` as an active log and never removes it, and the UI dump list only shows `.txt`/`.jfr`/`.bin`. Maybe `...-jcmd.txt`? - The benchmark section of the description predates the tool fixes (SASL mode, failed-call filtering). Could you refresh it? It becomes the commit message on squash. -- 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]
