Minor: Don’t send the DeleteTopicsRequest for the invalid topic names#4763
Conversation
|
cc @cmccabe |
|
LGTM. Thanks, @chia7712 |
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
Thanks for the PR. Left a couple comments.
There was a problem hiding this comment.
Maybe we could make this assertion precise? I think we expect the request count to be 0.
There was a problem hiding this comment.
Ya, the expected value of count is zero currently. However, we can't assume the initial count won't be changed in the future. For example, we plan to share the AdminClientUnitTestEnv (or MockClient) between all UTs to speedup the tests.
There was a problem hiding this comment.
Not sure I follow. What would be the benefit of sharing a MockClient?
There was a problem hiding this comment.
bq. What would be the benefit of sharing a MockClient?
Just saying (smile)
I'm fine to change the assertion.
There was a problem hiding this comment.
When do we decrement this?
There was a problem hiding this comment.
Does it need a way to decrement the count of a class used for testing?
There was a problem hiding this comment.
I may have misunderstood the intent. I thought it was meant to count in-flight requests, but seems it is a total count instead?
There was a problem hiding this comment.
bq. seems it is a total count instead?
yep. used only for testing so it should be fine without decrement call
There was a problem hiding this comment.
I think a total count is fine. Maybe name it totalRequestCount to make it obvious that that is what it is.
As @hachikuji commented, there is really no need to share a MockAdminClient across multiple tests. The mock client is extremely lightweight-- it doesn't manage any threads or sockets.
There was a problem hiding this comment.
Yes, totalRequestCount sounds clearer. We might also consider setting it back to 0 in MockClient.reset.
There was a problem hiding this comment.
Thanks for all reviews. Will address the following comments
- requestCount -> totalRequestCount
- set totalRequestCount to zero in MockClient.reset
27dc6dc to
029bc8d
Compare
…nt and reset totalRequestCount in MockClient.reset
029bc8d to
81130c2
Compare
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the patch!
…ache#4763) The invalid topic name is already handled locally so it is unnecessary to send the DeleteTopicsRequest. This PR adds a count to MockClient for testing. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jason Gustafson <jason@confluent.io>
3 participants
The invalid topic name is already handled locally so it is unnecessary to send the DeleteTopicsRequest. This PR adds a count to MockClient for testing.
Committer Checklist (excluded from commit message)