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
[2.2.x] Graceful Shutdown Refactoring #1154 #1347
[2.2.x] Graceful Shutdown Refactoring #1154 #1347
Conversation
ffb0010
to
748d184
Compare
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.
lgtm
shutdown(internalCaches); | ||
cachesToShutdown.addAll(icr.getInternalCacheNames()); | ||
cachesToShutdown.addAll(cacheManager.getCacheNames()); | ||
shutdown(cachesToShutdown); |
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.
Shouldn't we define the shutdown function before invoking it?
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.
Where the function is defined doesn't matter in Javascript. Executing this doesn't throw an error for me.
113aab9
to
d95edcf
Compare
@ryanemerson Would it make sense to update gracefulShutdown test to use 2 nodes? The test itself would uncover the issue if executed on OpenShift 4.9+ |
30ddfcf
to
304c89d
Compare
I've increased the number of nodes to 2, but the test won't fail unless we add a write to one of the caches that encounters the exception. Obviously I can't add that test to the CI until we have a new server image with the fix, so I think I'll create a follow up PR with this change |
1a73b3b
to
7458a85
Compare
config/manager/manager.yaml
Outdated
@@ -37,7 +37,7 @@ spec: | |||
fieldRef: | |||
fieldPath: metadata.namespace | |||
- name: RELATED_IMAGE_OPENJDK | |||
value: "quay.io/infinispan/server:13.0.1.Final" | |||
value: "quay.io/infinispan/server:13.0.2.Final" |
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.
Shouldn't this be 13.0.3?
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.
👍 It wasn't available when I created the PR and was required for CI. I'll be changing this to 13.0.3.Final as part of the release commit.
7458a85
to
bac098b
Compare
No longer explicitly delete pods on GracefulShutdown, instead scale the StatefulSet once all of the caches have been shutdown on the server.