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

isReady logs a warning when null #3307

Closed
shawkins opened this issue Jul 7, 2021 · 6 comments · Fixed by #3311
Closed

isReady logs a warning when null #3307

shawkins opened this issue Jul 7, 2021 · 6 comments · Fixed by #3311

Comments

@shawkins
Copy link
Contributor

shawkins commented Jul 7, 2021

If I do something like: pods().inNamespace...withName...isReady() - I'll see a warning log and a false result when null.

Should isReady return null when the resource is null?

@rohanKanojia
Copy link
Member

rohanKanojia commented Jul 7, 2021

I'm not sure about this, since this method is named isReady() user's expectations are set to receive a boolean value. I'm not sure if we should return null in this case.

@shawkins
Copy link
Contributor Author

shawkins commented Jul 7, 2021

The interface is already defined as Boolean isReady - if a null result is not possible, it should just be boolean.

@shawkins
Copy link
Contributor Author

shawkins commented Jul 7, 2021

The two proposals would be:

  1. return isReady null when the resource is null, with no associated logging. This could be seen by some users as a breaking change, but does conform to the existing interface.
  2. Change the interface to boolean isReady and throw a ResourceNotFoundException when the resource is null. This also would be a breaking change, but it would force the caller to handle the null situation rather than logging it.

@manusa
Copy link
Member

manusa commented Jul 9, 2021

I don't like the null return value, especially with your depicted scenario.

Users implementing (for whatever reason) a manual version of client.pods().inNamespace...withName...waitUntilReady(...), are probably expecting that statement to return false, since there is no resource available+ready matching that name+namespace.

So (IMHO) I see option 2 as the valid way, but excluding the ResourceNotFoundException

@shawkins
Copy link
Contributor Author

shawkins commented Jul 9, 2021

The root of the problem is:

logger.warn("{} is not a Readiable resource. It needs to be one of [{}]",

waitUntilReady does a null check before calling isReady(resource)

return waitUntilCondition(resource -> Objects.nonNull(resource) && getReadiness().isReady(resource), amount, timeUnit);

So you are saying just do the same for isReady() -

And update the interface boolean and javadocs to indicate that missing means not ready - I'm fine with that resolution as well. My impression from some of the logic that we wanted users to be aware that the value was null.

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Jul 10, 2021
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Jul 10, 2021
@shawkins shawkins linked a pull request Jul 10, 2021 that will close this issue
11 tasks
@manusa
Copy link
Member

manusa commented Jul 12, 2021

I specifically mentioned the waitUntilReady method because I'm aware that some users from 4.x versions might have a downstream implementation relying on the isReady method. But yes, my opinion is that both methods should be consistent.

I don't know why a Boolean was chosen as the return value instead of boolean in the first place. For me this is semantically confusing (thing are either ready or not), and might complicate things downstream.

Related to:

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 a pull request may close this issue.

3 participants