Skip to content

MINOR: InFlightRequests#isEmpty(node) method corrected.#3467

Closed
kamalcph wants to merge 4 commits into
apache:trunkfrom
kamalcph:frequest
Closed

MINOR: InFlightRequests#isEmpty(node) method corrected.#3467
kamalcph wants to merge 4 commits into
apache:trunkfrom
kamalcph:frequest

Conversation

@kamalcph
Copy link
Copy Markdown
Contributor

  • In clearAll method, get operation is removed.
  • variable name requestTimeout changed to requestTimeoutMs for clarity

@asfgit
Copy link
Copy Markdown

asfgit commented Jun 30, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5830/
Test PASSed (JDK 8 and Scala 2.12).

@asfgit
Copy link
Copy Markdown

asfgit commented Jun 30, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/5844/
Test PASSed (JDK 7 and Scala 2.11).

Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. LGTM

@asfgit asfgit closed this in 342f34a Jun 30, 2017
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 30, 2017

We should include a test to verify this change.

@rajinisivaram
Copy link
Copy Markdown
Contributor

@ijuma Unit tests of NetworkClient do check that InflightRequests works correctly. Obviously, it used to work before because two methods used matching wrong values, but I didn't think there was much value in unit tests for InflightRequests just for this change.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 30, 2017

@rajinisivaram Ideally we'd have unit tests for classes like InflightRequests. They make it less likely that things like this are missed. In this case, it was OK, but we could have had a situation where we started using the incorrect method in some edge case that wasn't exercised in the tests and it would be bad.

@rajinisivaram
Copy link
Copy Markdown
Contributor

@ijuma Ideally, we would have unit tests for all classes :-)
@kamal15 Would you like to submit unit test for InflightRequests?

@kamalcph
Copy link
Copy Markdown
Contributor Author

kamalcph commented Jul 1, 2017

@rajinisivaram @ijuma

Thanks for reviewing the PR. I ll submit the unit test for InflightRequests shortly.

@kamalcph kamalcph deleted the frequest branch August 25, 2017 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants