Eagerly deserialize responses consisting of list of data [API-1964] #618
Conversation
This file contains 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
We were performing lazy deserialization on APIs that return a list of data, such as map#values(). This was not going to work with Compact, as after returning the list to the user, there might be Compact serialized data on the list which we don't have the schema on the client yet. If it was a normal response, the client would have fetched the schema, but it is not possible with these lazy APIs. We perform the deserialization while getting the list items and this is a sync API. We can't perform a blocking call there, because there is a chance that this will be executed in the reactor thread, which would result in a deadlock. So, the only possible way of making these APIs work with Compact is to convert them to eager deserialization. This PR removes lazy deserialization in everything apart from SQL, which will be handled in another PR.
yuce
reviewed
Mar 23, 2023
hazelcast/util.py
Outdated
| @@ -175,6 +135,21 @@ def get_portable_version(portable, default_version): | |||
| return version | |||
|
|
|||
|
|
|||
| def deserialize_list_in_place(data_list, to_object_fn): | |||
There was a problem hiding this comment.
Should there be type annotation for the new functions?
There was a problem hiding this comment.
That makes sense, I wish we fully type hint the code base so that we can add checks for such things.
Codecov Report
@@ Coverage Diff @@
## master #618 +/- ##
=======================================
Coverage 96.48% 96.49%
=======================================
Files 357 357
Lines 20519 20554 +35
=======================================
+ Hits 19798 19833 +35
Misses 721 721
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
yuce
approved these changes
Mar 23, 2023
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.
We were performing lazy deserialization on APIs that return a list of data, such as map#values(). This was not going to work with Compact, as after returning the list to the user, there might be Compact serialized data on the list which we don't have the schema on the client yet. If it was a normal response, the client would have fetched the schema, but it is not possible with these lazy APIs. We perform the deserialization while getting the list items and this is a sync API. We can't perform a blocking call there, because there is a chance that this will be executed in the reactor thread, which would result in a deadlock.
So, the only possible way of making these APIs work with Compact is to convert them to eager deserialization.
This PR removes lazy deserialization in everything apart from SQL, which will be handled in another PR.