-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-4266: ReassignPartitionsClusterTest - Ensure ZK publication completed before start #1997
Conversation
AdminUtils.changeBrokerConfig(zkUtils, Seq(id), configs) | ||
} | ||
Thread.sleep(throttle.postUpdateDelay) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please elaborate a little on which scenario is made more reliable by this sleep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the rebalance kicks in before the throttle is set data isn't replicated. The current solution is to just make the test run for longer, with more data, so it averages out.
It's in response to this commit: 5e4600a
We too are experiencing fragility in ReassignPartitionsClusterTest and this PR seems to address the fragile test. Are there any plans of merging this PR? |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@silpamittapalli, have you verified that this fixes the issues you are seeing? |
Yes, it addresses the fragile test @ijuma |
happy to rebase if @ijuma has time to review? |
@benstopford, yes, I can review it tomorrow. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
7e9f0e9
to
aefd304
Compare
@ijuma rebased dude |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
import org.apache.kafka.common.utils.Utils | ||
import org.apache.kafka.common.security.JaasUtils | ||
|
||
object ReassignPartitionsCommand extends Logging { | ||
|
||
case class Throttle (value: Long, postUpdateDelay: Int = 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor nit: could we take a function postUpdate: () => Unit
and then do the sleep in the test? That seems a bit nicer than having the sleep in the command. Apart from that, this looks OK, I could not think of a better way to detect that the throttling information had propagated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Done
configs.put(DynamicConfig.Broker.LeaderReplicationThrottledRateProp, throttle.toString) | ||
configs.put(DynamicConfig.Broker.FollowerReplicationThrottledRateProp, throttle.toString) | ||
configs.put(DynamicConfig.Broker.LeaderReplicationThrottledRateProp, throttle.value.toString) | ||
configs.put(DynamicConfig.Broker.FollowerReplicationThrottledRateProp, throttle.value.toString) | ||
admin.changeBrokerConfig(zkUtils, Seq(id), configs) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other question, should the delay be here or after assignThrottledReplicas
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - switched the order so all config is updated before the post update action. Considered putting a post update action into each method but it seems like more noise so I think this is the best balance.
@ijuma changes made as requested. |
…ted before proceeding with the test (where applicable)
- Pulled out sleep into test using a post update action. - Reordered config updates so sleep is at the end.
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
|
||
package kafka.admin | ||
|
||
case class Throttle (value: Long, action: PostUpdateAction = NoPostUpdateAction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you used a PostUpdateAction
class instead of just a function? e.g.:
case class Throttle(value: Long, postUpdateAction: () => Unit = () => ())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Laziness (i needed a class at an earlier point).
@ijuma Removed the redundant class. Thanks for the review. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Thanks, LGTM. I did a few minor tweaks before merging. |
Refer to this link for build results (access rights to CI server needed): |
Increase the reliability of the one temporal comparison in ReassignPartitionsClusterTest by imposing a delay after ZK is updated. This should be more reliable than just increasing the amount of data.
This relates to a previous PR: #1982