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.

Sign up for GitHub

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

Jump to bottom

Add a grpc streaming helper #4

Merged
merged 4 commits into from
Dec 18, 2023
Merged

Add a grpc streaming helper #4

merged 4 commits into from
Dec 18, 2023

Conversation

shsms
Copy link
Contributor

@shsms shsms commented Dec 15, 2023 •
edited
Loading

This PR extracts the streaming code from the SDK that converts grpc streams into channel receivers, into the generic GrpcStreamingHelper.

The RetryStrategy implementation is also extracted from the SDK.

@shsms
Remove repo-config tests
d46c043 
Signed-off-by: Sahas Subramanian <[email protected]>
@shsms
Import retry strategies from the SDK
2564e88 
Signed-off-by: Sahas Subramanian <[email protected]>
@shsms shsms requested a review from a team as a code owner December 15, 2023 13:06
@shsms shsms requested review from Marenz and removed request for a team December 15, 2023 13:06
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) labels Dec 15, 2023
llucax
llucax reviewed Dec 15, 2023
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just gave it a quick look. Besides the extra directory levels and considering this is code being moved from somewhere else (so I don't want to do a full review of the code because it is probably a bit out of scope for this PR), LGTM.

src/frequenz/client/base/grpc_streaming_helper/_grpc_streaming_helper.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to put this directly in src/frequenz/client/base/grpc_streaming_helper.py instead of having a subdirectory with an extra __init__.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/frequenz/client/base/grpc_streaming_helper/_grpc_streaming_helper.py Outdated


class GrpcStreamingHelper(typing.Generic[_InputT, _OutputT]):
"""Helper class to handle grpc streaming methods."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little more docs with an example would be nice to have.

src/frequenz/client/base/retry_strategy/_retry.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, why not just src/frequenz/client/base/retry_strategy.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that file has a bunch of not-underscore symbols that I didn't want to expose, let me do that in a new commit. Also need to add a NoRetry strategy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/frequenz/client/base/grpc_streaming_helper/_grpc_streaming_helper.py Outdated
self._stream_method = stream_method
self._transform = transform
self._retry_spec = (
retry_strategy.LinearBackoff() if retry_spec is None else retry_spec.copy()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a comment to suggest to make retry specs/strategy immutable, so we don't need to copy them. But now I see retry strategies are actually keeping state, so it is not really a spec or a strategy, is more like a tracker, that you need to reset when a connection is complete, so the copy makes sense, but it might be nice to look for a clearer name in the future (in a different PR), RetryTracker or something more along those lines.

llucax
llucax approved these changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not putting the focus on existing code but just on structure in the new repo.

@shsms
Add a grpc streaming helper
bb79be8 
Signed-off-by: Sahas Subramanian <[email protected]>
@shsms
Update RELEASE_NOTES.md
29c0c6e 
Signed-off-by: Sahas Subramanian <[email protected]>
@github-actions github-actions bot added the part:docs Affects the documentation label Dec 18, 2023
@shsms
Copy link
Contributor Author

shsms commented Dec 18, 2023

I've squashed the commits and updated release notes.

@shsms shsms merged commit 039d4af into frequenz-floss:v0.x.x Dec 18, 2023
14 checks passed
@shsms shsms deleted the streaming-helper branch December 18, 2023 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llucax llucax llucax approved these changes

@Marenz Marenz Awaiting requested review from Marenz Marenz is a code owner automatically assigned from frequenz-floss/python-sdk-team

Assignees
No one assigned
Labels
part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shsms @llucax