Add support for setting a maximum session age in RemoteModuleBase #761
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
gabe-l-hart
requested changes
Aug 27, 2024
Comment on lines
483
to
489
| # If the max session age is less then 0 then reuse the existing con channel | ||
| elif self._connection.max_session_age < 0: | ||
| pass | ||
|
|
||
| # If the current time is greater than the conn channel timeout then construct a | ||
| # new channel | ||
| elif datetime.now() - self._current_conn_time > self._max_conn_delta: |
There was a problem hiding this comment.
The elif...pass is a bit confusing. I'd suggest merging these into a single elif
There was a problem hiding this comment.
Good call, I was trying to be explicit but just made it more confusing
| def test_remote_initializer_always_new_channel( | ||
| sample_task_model_id, open_port, protocol | ||
| ): | ||
| """Test to ensure RemoteModule Initializer negative max connection age always creates |
There was a problem hiding this comment.
Suggested change
| """Test to ensure RemoteModule Initializer negative max connection age always creates | |
| """Test to ensure RemoteModule Initializer zero max connection age always creates |
| remote_model = remote_initializer.init(remote_config) | ||
| assert isinstance(remote_model, ModuleBase) | ||
|
|
||
| # Run RemoteModule Request twice to ensure the channel is retained. Run the second one with a small sleep to ensure the third request gets a new object |
There was a problem hiding this comment.
We need a new definition of "small sleep!" I worry this is a little error prone with sleep-based behavior. Could we use a mock to patch over the time.time function to make it appear as "timed out" on the third call?
There was a problem hiding this comment.
Good call on mock! Done
gabe-l-hart
approved these changes
Aug 28, 2024
Signed-off-by: Michael Honaker <Michael.Honaker@ibm.com>
9675378 to
35650b3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This PR adds support for setting a maximum session/channel age in the RemoteModuleBase. This is primarily useful for grpc connections which only poll dns once on creation. This means that if you add a downstream server (like Kubernetes HPA) to the DNS pool existing grpc channels won't "discover" the new replicas until the pod has been restarted.
Special notes for your reviewer:
If applicable: