fix #3615: always requesting bookmarks #3617
Conversation
|
Can one of the admins verify this patch? |
|
@manusa @rohanKanojia other than needing to correct a lot of tests, is there any reason why we shouldn't automatically request bookmarks? I see our watch logic as being the same as a retry watcher, so it really should support this by default. |
|
@manusa after some discussion we decided that opt into bookmarks by default seems reasonable. This can be considered for inclusion in 5.11, I'll deal with any test conflicts with the okhttp refactoring later. |
| @@ -178,6 +185,10 @@ boolean isForceClosed() { | |||
| } | |||
|
|
|||
| void eventReceived(Watcher.Action action, HasMetadata resource) { | |||
| if (!receiveBookmarks && action == Action.BOOKMARK) { | |||
There was a problem hiding this comment.
Is that all that's required to support bookmarks? What is needed by watchers to handle bookmark events?
There was a problem hiding this comment.
If a user has not explicitly set allowWatchBookmarks=true in their ListOptions, then this will filter the events before they reach the Watcher. Otherwise the built-in handling is already there in AbstractWatchManager.onMessage - we just need updateResourceVersion to be called on a bookmark.
There was a problem hiding this comment.
So will that update of the resource version be part of a separate PR or will it be done once the status of the okhttp PR is settled?
There was a problem hiding this comment.
It's already there, this is what I was referring to:
bookmarks # Conflicts: # CHANGELOG.md
| @@ -39,8 +39,8 @@ | |||
| import java.util.concurrent.CountDownLatch; | |||
| import java.util.concurrent.TimeUnit; | |||
|
|
|||
| import static io.fabric8.kubernetes.client.Watcher.Action.DELETED; | |||
There was a problem hiding this comment.
Minor: Maybe have these imports ordering changes in a separate changeset/commit (or not including them to the PR)
There was a problem hiding this comment.
I'd opt for leaving this change. I know imports can be a touchy subject. If devs are open to it I can add automatic import ordering using the the impsort-maven-plugin, just like is done with quarkus.
| //We need to connect the pipe here, because onResponse might not be called in time (if log is empty) | ||
| //This will cause a `Pipe not connected` exception for everyone that tries to read. By always opening | ||
| //the pipe the user will get a ready to use inputstream, which will block until there is actually something to read. | ||
| if (this.out instanceof PipedOutputStream && this.output != null) { | ||
| try { |
There was a problem hiding this comment.
I am not sure if this is related to always enable bookmarks. Could you tell us more about these changes ?
There was a problem hiding this comment.
No, this came in unintentionally and are part of another pr.
|
There are some conflicts which need to be resolved |
bookmarks # Conflicts: # CHANGELOG.md # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/AbstractWatchManager.java # kubernetes-client/src/test/java/io/fabric8/kubernetes/client/dsl/internal/RawCustomResourceOperationsImplTest.java
Should be good now. |
|
Thanks a lot, let's wait for Marc to approve then we can proceed with merge. |
|
SonarCloud Quality Gate failed. |
Description
Draft fix for #3615 - always enable bookmarks since we always implicitly retry watches.
Since this affects a great number of test results, some of which are also touched by the okhttp refactoring, I'll pick this back up after that is committed.
Type of change
test, version modification, documentation, etc.)
Checklist