Skip to content

Replace UtilWaitThread.sleepUninterruptibly with guava equivalent#3121

Merged
DomGarguilo merged 2 commits into
apache:mainfrom
DomGarguilo:removeSleepUninteruptibly
Dec 16, 2022
Merged

Replace UtilWaitThread.sleepUninterruptibly with guava equivalent#3121
DomGarguilo merged 2 commits into
apache:mainfrom
DomGarguilo:removeSleepUninteruptibly

Conversation

@DomGarguilo

Copy link
Copy Markdown
Member

UtilWaitThread.sleepUninterruptibly was copied from guavas Uninterruptibles.sleepUninterruptibly when it was still tagged as beta. With the version of guava we are using, Uninterruptibles.sleepUninterruptibly is no longer tagged as beta. This PR removes the copied method and uses guava directly.

@DomGarguilo DomGarguilo self-assigned this Dec 12, 2022
@EdColeman

Copy link
Copy Markdown
Contributor

Are there any newer java 11 added options instead of guava?

@dlmarion

Copy link
Copy Markdown
Contributor

I thought we were trying to remove Guava dependencies in general.

@cshannon

cshannon commented Dec 14, 2022

Copy link
Copy Markdown
Contributor

It definitely makes sense to me to first try and use native Java code/utilities when possible, such as lambdas instead of Guavas iterable utilities. But if something doesn't exist I think it would be fine to use the utility in Guava or something like a Commons library so we don't have to re-invent the wheel, assuming the plan isn't to actually remove the dependency entirely at some point.

@DomGarguilo

Copy link
Copy Markdown
Member Author

It definitely makes sense to me to first try and use native Java code/utilities when possible, such as lambdas instead of Guavas iterable utilities. But if something doesn't exist I think it would be fine to use the utility in Guava or something like a Commons library so we don't have to re-invent the wheel, assuming the plan isn't to actually remove the dependency entirely at some point.

I agree with these points.

@EdColeman I don't know of/can't find any native java replacements for this. That's not to say one does not exist but I haven't been able to find anything yet.

@dlmarion As for removing guava, I am not sure either. My understanding is that when possible, we should avoid using guava for a built in equivalent, not sure if we have plans to completely remove it as a dependency though.

@ctubbsii

Copy link
Copy Markdown
Member

I don't think we have a plan to remove Guava entirely. We have tried to avoid the problematic beta methods, certainly. It's also good to reduce our dependency on any external library, Guava is not an exception to that. The modernizer-maven-plugin helps identify some of those things. For non-beta methods that we expect to be stable, and for which we expect a minimum Guava version to be required, I don't think it's a problem to use the utility that Guava provides. We'll need to fix the dependency check first before this is merged (just add the guava dependency explicitly to the modules that didn't have it as a compile time dependency before, but now do).

@EdColeman

Copy link
Copy Markdown
Contributor

Is the build failure transient - or is there a dependency that needs to be adjusted?

@DomGarguilo

Copy link
Copy Markdown
Member Author

Is the build failure transient - or is there a dependency that needs to be adjusted?

No, it isn't transient. The build is passing from a "used undeclared" dependency warning. Looking into solutions now - should have a fix up soon.

@DomGarguilo

Copy link
Copy Markdown
Member Author

Is the build failure transient - or is there a dependency that needs to be adjusted?

Fixed in ac427d0 @EdColeman

@cshannon
cshannon self-requested a review December 16, 2022 11:43

@cshannon cshannon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems fine to me now that the checks are passing unless anyone else has any comments or concerns.

@DomGarguilo
DomGarguilo merged commit 6712d25 into apache:main Dec 16, 2022
@DomGarguilo
DomGarguilo deleted the removeSleepUninteruptibly branch December 16, 2022 13:50
@ctubbsii ctubbsii added this to the 3.0.0 milestone Jul 12, 2024
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.

5 participants