xiangfu0 commented on PR #12292: URL: https://github.com/apache/pinot/pull/12292#issuecomment-1907054267
> LGTM. A couple of comments though: > > 1. Can you please update the main class documentation to reflect the new addition? > 2. The additional logic is straight-forward. Do we really need an integration test? Having more tests in general is good, but this test adds a few minutes to the runtime of the test suits, and I'm not sure if it's worth incurring the cost. 1. will do 2. I feel it's necessary for this one and we may want to add more tests later on for bytes-based throttling etc. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
