Skip to content
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

Use Awaitility to instead of Thread sheep method. #389

Merged
merged 2 commits into from Jul 12, 2021

Conversation

mattisonchao
Copy link
Member

  • Use Awaitility to avoid busy waiting.
  • Refactor some code to Java8

- Use awaitility to avoid busy waiting.
- Refactor some code to Java8
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

we are using a lot Awaiatility in Apache Pulsar.

would you like to follow up and enhance more tests ?

@eolivelli
Copy link
Contributor

@tisonkun @Randgalt @cammckenzie
if you have time please take a look, the change is simple

I like Awaitility, we started to use it in Pulsar in order to make tests code cleaner and in general reduce flakyness

@mattisonchao
Copy link
Member Author

LGTM

we are using a lot Awaiatility in Apache Pulsar.

would you like to follow up and enhance more tests ?

Sure, I will do that.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your work!

I'd suggest separate a commit to refactor lambda expression for your next contributions @mattisonchao

{
Thread.sleep(100);
}
Awaitility.await()
Copy link
Contributor

Choose a reason for hiding this comment

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

here you are adding an implicit timeout of 10 seconds
I am fine with it, as far as it does not make the test become flaky

@eolivelli eolivelli merged commit 4dffb5e into apache:master Jul 12, 2021
@eolivelli
Copy link
Contributor

@mattisonchao I have merged this PR.

but in Curator we use JIRA to track the work

can you please create a ticket here and link it to this PR ?
https://issues.apache.org/jira/secure/Dashboard.jspa

Usually we require that the commit message reports the JIRA ticket ID.

@tisonkun I was too quick in using the Git Hub merge button :(
we know have a script in Curator that prevents us from doing this kind of errors while committing PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants