[GitHub] [kafka] soondenana commented on pull request #9347: KAFKA-10531: Check for negative values to Thread.sleep call

[GitBox <gi...@apache.org>]

soondenana commented on pull request #9347:
URL: https://github.com/apache/kafka/pull/9347#issuecomment-702938936


   > @soondenana if the goal is to minimize the changes, would it be sufficient to change the code to use `Time.sleep(...)` instead of `Thread.sleep(...)`, and then change the `SystemTime.sleep(...)` implementation to return immediately if the supplied number of milliseconds is <= 0?
   > 
   > Are there performance implications of using `System.nanoTime()` instead of `System.currentTimeMillis()`?
   
   Thanks @rhauch for taking a look. The PR started with only fixing negative sleep vlaue, but as we started looking at code more, there were multiple issue so the "minimize change" idea was dropped. Here are 3 issues with original code:
   
   1. Negative sleep value
   2. Using elapsed time since loop started to decide on next sleep time.
   3. Sleeping even if the `partitionsFor` call would be successful for first time
   
   Considering that I decided to rewrite the loop to fix all three issues. 
   
   Good question on performance. It seems like `System.nanoTime()` is slower than `System.currentTimeMillis` (one is read from h/w using i/o another one is reading from memory), but not sure if that has any implication here for this use case. We are waiting for topic to appear in metadata and sleeping for seconds. Few milliseconds difference should not matter.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org