{"action":"created","comment":{"url":"https://api.github.com/repos/apache/kafka/pulls/comments/42420384","id":42420384,"diff_hunk":"@@ -149,12 +149,25 @@ class LogCleaner(val config: CleanerConfig,\n   }\n \n   /**\n-   * For testing, a way to know when work has completed. This method blocks until the\n+   * For testing, a way to know when work has completed. This method waits until the\n    * cleaner has processed up to the given offset on the specified topic/partition\n+   *\n+   * @param topic The Topic to be cleaned\n+   * @param part The partition of the topic to be cleaned\n+   * @param offset The first dirty offset that the cleaner doesn't have to clean\n+   * @param maxWaitMs The maximum time in ms to wait for cleaner\n+   *\n+   * @return A boolean indicating whether the work has completed before timeout\n    */\n-  def awaitCleaned(topic: String, part: Int, offset: Long, timeout: Long = 30000L): Unit = {\n-    while(!cleanerManager.allCleanerCheckpoints.contains(TopicAndPartition(topic, part)))\n-      Thread.sleep(10)\n+  def awaitCleaned(topic: String, part: Int, offset: Long, maxWaitMs: Long = 60000L): Boolean = {","path":"core/src/main/scala/kafka/log/LogCleaner.scala","position":18,"original_position":18,"commit_id":"0f4d1cf9424988a78d84f2c1f2aede3dc069c181","original_commit_id":"6cbb4a559b80979b4a5789beee26a38e5fb55436","user":{"login":"lindong28","id":5873403,"avatar_url":"https://avatars.githubusercontent.com/u/5873403?v=3","gravatar_id":"","url":"https://api.github.com/users/lindong28","html_url":"https://github.com/lindong28","followers_url":"https://api.github.com/users/lindong28/followers","following_url":"https://api.github.com/users/lindong28/following{/other_user}","gists_url":"https://api.github.com/users/lindong28/gists{/gist_id}","starred_url":"https://api.github.com/users/lindong28/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lindong28/subscriptions","organizations_url":"https://api.github.com/users/lindong28/orgs","repos_url":"https://api.github.com/users/lindong28/repos","events_url":"https://api.github.com/users/lindong28/events{/privacy}","received_events_url":"https://api.github.com/users/lindong28/received_events","type":"User","site_admin":false},"body":"I think the first problem is minor and it is OK to change it twice. But I agree it is a valid problem. In stead of following either of our approach, I think it is better to do `sleepInterval = 100` to make this an explicit configuration.\r\n\r\nI am a bit confused by your comments on the 2nd problem... I had this implementation previously that doesn't have this problem:\r\n\r\n```\r\n  def awaitCleaned(topic: String, part: Int, offset: Long, maxWaitMs: Long = 60000L): Boolean = {\r\n    def isCleaned = cleanerManager.allCleanerCheckpoints.get(TopicAndPartition(topic, part)).fold(false)(_ >= offset)\r\n    var remainingWaitMs = maxWaitMs\r\n    while (!isCleaned && remainingWaitMs > 0) {\r\n      Thread.sleep(math.min(100, remainingWaitMs))\r\n      remainingWaitMs = math.max(0, remainingWaitMs - 100)\r\n    }\r\n    return isCleaned\r\n  }\r\n```\r\n\r\nThen you made the following comment:\r\n\r\n```\r\nGreat, thanks. Sorry to nitpick, but you don't need the math.max in remainingWaitMs = ... since we check for > 0 (that is, it's OK for the value to be negative). Looks good otherwise.\r\n```\r\n\r\nTherefore I change the implementation to do `remainingWaitMs -= 100`. Actually I am fine with both solution. But now I am confused by your latest comment that \"it is not OK to have remainingWaitMs < 0\". Did I misunderstand your comment?","created_at":"2015-10-19T20:18:31Z","updated_at":"2015-10-19T20:18:31Z","html_url":"https://github.com/apache/kafka/pull/327#discussion_r42420384","pull_request_url":"https://api.github.com/repos/apache/kafka/pulls/327","_links":{"self":{"href":"https://api.github.com/repos/apache/kafka/pulls/comments/42420384"},"html":{"href":"https://github.com/apache/kafka/pull/327#discussion_r42420384"},"pull_request":{"href":"https://api.github.com/repos/apache/kafka/pulls/327"}}},"pull_request":{"url":"https://api.github.com/repos/apache/kafka/pulls/327","id":47983162,"html_url":"https://github.com/apache/kafka/pull/327","diff_url":"https://github.com/apache/kafka/pull/327.diff","patch_url":"https://github.com/apache/kafka/pull/327.patch","issue_url":"https://api.github.com/repos/apache/kafka/issues/327","number":327,"state":"open","locked":false,"title":"KAFKA-2669; Fix LogCleanerIntegrationTest","user":{"login":"lindong28","id":5873403,"avatar_url":"https://avatars.githubusercontent.com/u/5873403?v=3","gravatar_id":"","url":"https://api.github.com/users/lindong28","html_url":"https://github.com/lindong28","followers_url":"https://api.github.com/users/lindong28/followers","following_url":"https://api.github.com/users/lindong28/following{/other_user}","gists_url":"https://api.github.com/users/lindong28/gists{/gist_id}","starred_url":"https://api.github.com/users/lindong28/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lindong28/subscriptions","organizations_url":"https://api.github.com/users/lindong28/orgs","repos_url":"https://api.github.com/users/lindong28/repos","events_url":"https://api.github.com/users/lindong28/events{/privacy}","received_events_url":"https://api.github.com/users/lindong28/received_events","type":"User","site_admin":false},"body":"LogCleanerIntegrationTest calls LogCleaner.awaitCleaned() to wait until cleaner has processed up to given offset. However, existing awaitCleaned() implementation doesn't wait for this. This patch fix the problem.","created_at":"2015-10-17T09:18:16Z","updated_at":"2015-10-19T20:18:31Z","closed_at":null,"merged_at":null,"merge_commit_sha":"722d05674d1c5b20139505849175212a53f20eed","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/apache/kafka/pulls/327/commits","review_comments_url":"https://api.github.com/repos/apache/kafka/pulls/327/comments","review_comment_url":"https://api.github.com/repos/apache/kafka/pulls/comments{/number}","comments_url":"https://api.github.com/repos/apache/kafka/issues/327/comments","statuses_url":"https://api.github.com/repos/apache/kafka/statuses/0f4d1cf9424988a78d84f2c1f2aede3dc069c181","head":{"label":"lindong28:KAFKA-2669","ref":"KAFKA-2669","sha":"0f4d1cf9424988a78d84f2c1f2aede3dc069c181","user":{"login":"lindong28","id":5873403,"avatar_url":"https://avatars.githubusercontent.com/u/5873403?v=3","gravatar_id":"","url":"https://api.github.com/users/lindong28","html_url":"https://github.com/lindong28","followers_url":"https://api.github.com/users/lindong28/followers","following_url":"https://api.github.com/users/lindong28/following{/other_user}","gists_url":"https://api.github.com/users/lindong28/gists{/gist_id}","starred_url":"https://api.github.com/users/lindong28/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lindong28/subscriptions","organizations_url":"https://api.github.com/users/lindong28/orgs","repos_url":"https://api.github.com/users/lindong28/repos","events_url":"https://api.github.com/users/lindong28/events{/privacy}","received_events_url":"https://api.github.com/users/lindong28/received_events","type":"User","site_admin":false},"repo":{"id":21054638,"name":"kafka","full_name":"lindong28/kafka","owner":{"login":"lindong28","id":5873403,"avatar_url":"https://avatars.githubusercontent.com/u/5873403?v=3","gravatar_id":"","url":"https://api.github.com/users/lindong28","html_url":"https://github.com/lindong28","followers_url":"https://api.github.com/users/lindong28/followers","following_url":"https://api.github.com/users/lindong28/following{/other_user}","gists_url":"https://api.github.com/users/lindong28/gists{/gist_id}","starred_url":"https://api.github.com/users/lindong28/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lindong28/subscriptions","organizations_url":"https://api.github.com/users/lindong28/orgs","repos_url":"https://api.github.com/users/lindong28/repos","events_url":"https://api.github.com/users/lindong28/events{/privacy}","received_events_url":"https://api.github.com/users/lindong28/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/lindong28/kafka","description":"Mirror of Apache Kafka","fork":true,"url":"https://api.github.com/repos/lindong28/kafka","forks_url":"https://api.github.com/repos/lindong28/kafka/forks","keys_url":"https://api.github.com/repos/lindong28/kafka/keys{/key_id}","collaborators_url":"https://api.github.com/repos/lindong28/kafka/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/lindong28/kafka/teams","hooks_url":"https://api.github.com/repos/lindong28/kafka/hooks","issue_events_url":"https://api.github.com/repos/lindong28/kafka/issues/events{/number}","events_url":"https://api.github.com/repos/lindong28/kafka/events","assignees_url":"https://api.github.com/repos/lindong28/kafka/assignees{/user}","branches_url":"https://api.github.com/repos/lindong28/kafka/branches{/branch}","tags_url":"https://api.github.com/repos/lindong28/kafka/tags","blobs_url":"https://api.github.com/repos/lindong28/kafka/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/lindong28/kafka/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/lindong28/kafka/git/refs{/sha}","trees_url":"https://api.github.com/repos/lindong28/kafka/git/trees{/sha}","statuses_url":"https://api.github.com/repos/lindong28/kafka/statuses/{sha}","languages_url":"https://api.github.com/repos/lindong28/kafka/languages","stargazers_url":"https://api.github.com/repos/lindong28/kafka/stargazers","contributors_url":"https://api.github.com/repos/lindong28/kafka/contributors","subscribers_url":"https://api.github.com/repos/lindong28/kafka/subscribers","subscription_url":"https://api.github.com/repos/lindong28/kafka/subscription","commits_url":"https://api.github.com/repos/lindong28/kafka/commits{/sha}","git_commits_url":"https://api.github.com/repos/lindong28/kafka/git/commits{/sha}","comments_url":"https://api.github.com/repos/lindong28/kafka/comments{/number}","issue_comment_url":"https://api.github.com/repos/lindong28/kafka/issues/comments{/number}","contents_url":"https://api.github.com/repos/lindong28/kafka/contents/{+path}","compare_url":"https://api.github.com/repos/lindong28/kafka/compare/{base}...{head}","merges_url":"https://api.github.com/repos/lindong28/kafka/merges","archive_url":"https://api.github.com/repos/lindong28/kafka/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/lindong28/kafka/downloads","issues_url":"https://api.github.com/repos/lindong28/kafka/issues{/number}","pulls_url":"https://api.github.com/repos/lindong28/kafka/pulls{/number}","milestones_url":"https://api.github.com/repos/lindong28/kafka/milestones{/number}","notifications_url":"https://api.github.com/repos/lindong28/kafka/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/lindong28/kafka/labels{/name}","releases_url":"https://api.github.com/repos/lindong28/kafka/releases{/id}","created_at":"2014-06-20T22:40:57Z","updated_at":"2014-06-20T22:40:58Z","pushed_at":"2015-10-19T17:22:13Z","git_url":"git://github.com/lindong28/kafka.git","ssh_url":"git@github.com:lindong28/kafka.git","clone_url":"https://github.com/lindong28/kafka.git","svn_url":"https://github.com/lindong28/kafka","homepage":null,"size":23165,"stargazers_count":0,"watchers_count":0,"language":"Scala","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"0.8.1"}},"base":{"label":"apache:trunk","ref":"trunk","sha":"78a2e2f8f39b24774e5edc8cf0c1093889db2fdf","user":{"login":"apache","id":47359,"avatar_url":"https://avatars.githubusercontent.com/u/47359?v=3","gravatar_id":"","url":"https://api.github.com/users/apache","html_url":"https://github.com/apache","followers_url":"https://api.github.com/users/apache/followers","following_url":"https://api.github.com/users/apache/following{/other_user}","gists_url":"https://api.github.com/users/apache/gists{/gist_id}","starred_url":"https://api.github.com/users/apache/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/apache/subscriptions","organizations_url":"https://api.github.com/users/apache/orgs","repos_url":"https://api.github.com/users/apache/repos","events_url":"https://api.github.com/users/apache/events{/privacy}","received_events_url":"https://api.github.com/users/apache/received_events","type":"Organization","site_admin":false},"repo":{"id":2211243,"name":"kafka","full_name":"apache/kafka","owner":{"login":"apache","id":47359,"avatar_url":"https://avatars.githubusercontent.com/u/47359?v=3","gravatar_id":"","url":"https://api.github.com/users/apache","html_url":"https://github.com/apache","followers_url":"https://api.github.com/users/apache/followers","following_url":"https://api.github.com/users/apache/following{/other_user}","gists_url":"https://api.github.com/users/apache/gists{/gist_id}","starred_url":"https://api.github.com/users/apache/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/apache/subscriptions","organizations_url":"https://api.github.com/users/apache/orgs","repos_url":"https://api.github.com/users/apache/repos","events_url":"https://api.github.com/users/apache/events{/privacy}","received_events_url":"https://api.github.com/users/apache/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/apache/kafka","description":"Mirror of Apache Kafka","fork":false,"url":"https://api.github.com/repos/apache/kafka","forks_url":"https://api.github.com/repos/apache/kafka/forks","keys_url":"https://api.github.com/repos/apache/kafka/keys{/key_id}","collaborators_url":"https://api.github.com/repos/apache/kafka/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/apache/kafka/teams","hooks_url":"https://api.github.com/repos/apache/kafka/hooks","issue_events_url":"https://api.github.com/repos/apache/kafka/issues/events{/number}","events_url":"https://api.github.com/repos/apache/kafka/events","assignees_url":"https://api.github.com/repos/apache/kafka/assignees{/user}","branches_url":"https://api.github.com/repos/apache/kafka/branches{/branch}","tags_url":"https://api.github.com/repos/apache/kafka/tags","blobs_url":"https://api.github.com/repos/apache/kafka/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/apache/kafka/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/apache/kafka/git/refs{/sha}","trees_url":"https://api.github.com/repos/apache/kafka/git/trees{/sha}","statuses_url":"https://api.github.com/repos/apache/kafka/statuses/{sha}","languages_url":"https://api.github.com/repos/apache/kafka/languages","stargazers_url":"https://api.github.com/repos/apache/kafka/stargazers","contributors_url":"https://api.github.com/repos/apache/kafka/contributors","subscribers_url":"https://api.github.com/repos/apache/kafka/subscribers","subscription_url":"https://api.github.com/repos/apache/kafka/subscription","commits_url":"https://api.github.com/repos/apache/kafka/commits{/sha}","git_commits_url":"https://api.github.com/repos/apache/kafka/git/commits{/sha}","comments_url":"https://api.github.com/repos/apache/kafka/comments{/number}","issue_comment_url":"https://api.github.com/repos/apache/kafka/issues/comments{/number}","contents_url":"https://api.github.com/repos/apache/kafka/contents/{+path}","compare_url":"https://api.github.com/repos/apache/kafka/compare/{base}...{head}","merges_url":"https://api.github.com/repos/apache/kafka/merges","archive_url":"https://api.github.com/repos/apache/kafka/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/apache/kafka/downloads","issues_url":"https://api.github.com/repos/apache/kafka/issues{/number}","pulls_url":"https://api.github.com/repos/apache/kafka/pulls{/number}","milestones_url":"https://api.github.com/repos/apache/kafka/milestones{/number}","notifications_url":"https://api.github.com/repos/apache/kafka/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/apache/kafka/labels{/name}","releases_url":"https://api.github.com/repos/apache/kafka/releases{/id}","created_at":"2011-08-15T18:06:16Z","updated_at":"2015-10-19T10:43:46Z","pushed_at":"2015-10-19T18:50:13Z","git_url":"git://github.com/apache/kafka.git","ssh_url":"git@github.com:apache/kafka.git","clone_url":"https://github.com/apache/kafka.git","svn_url":"https://github.com/apache/kafka","homepage":null,"size":37367,"stargazers_count":2072,"watchers_count":2072,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":1229,"mirror_url":"git://git.apache.org/kafka.git","open_issues_count":95,"forks":1229,"open_issues":95,"watchers":2072,"default_branch":"trunk"}},"_links":{"self":{"href":"https://api.github.com/repos/apache/kafka/pulls/327"},"html":{"href":"https://github.com/apache/kafka/pull/327"},"issue":{"href":"https://api.github.com/repos/apache/kafka/issues/327"},"comments":{"href":"https://api.github.com/repos/apache/kafka/issues/327/comments"},"review_comments":{"href":"https://api.github.com/repos/apache/kafka/pulls/327/comments"},"review_comment":{"href":"https://api.github.com/repos/apache/kafka/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/apache/kafka/pulls/327/commits"},"statuses":{"href":"https://api.github.com/repos/apache/kafka/statuses/0f4d1cf9424988a78d84f2c1f2aede3dc069c181"}}}}