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.

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

Make GRPC transport decorators configurable & disable atomic decorator by default #1388

Merged
merged 4 commits into from
Feb 19, 2025

Conversation

bsenthilr
Copy link
Collaborator

@bsenthilr bsenthilr commented Feb 18, 2025

Problem statement

The atomic connection decorators always reestablish connections on transport IO. This is not a desired IO characteristic especially due to penalty of reconnecting.
This PR proposes configuration to make these decorators option. By default atomic connections are disabled and can be provisioned when required.

Background information on atomic connections (from @psfoley)

The background of reestablishing a connection for every request was due to early issues. There were problems resulting from long established connections when there was any kind of network instability. This started happening more with certain releases of gRPC around that time (1.32?).
The atomic connection decorator, and establishing the connection only when communication was needed, was the solution for that.
As long as the retry logic is left intact, I don't expect there would be problems making the change back to long standing connections (as long as this is thoroughly tested)

Summary of changes

  • Add knobs to provision these decorators optionally when required.
  • Log connection points since this is critical for debugging and tuning other attributes of service such as rate limiting etc.,

Testing done

  • Verified federation run with atomic connectivity disabled hence eliminating service level reconnects.
  • Verified federation run with atomic connectivity enabled and correlating reconnects with logs.

@bsenthilr
Copy link
Collaborator Author

Log snippets with atomic connects:

[collaborator1]	           INFO     Waiting for tasks...                                                                                                     collaborator.py:233
[collaborator1]	           INFO     Disconnecting from gRPC server at **redacted**                                                aggregator_client.py:365
[collaborator1]	           INFO     Connecting to gRPC at **redacted**                                                            aggregator_client.py:385

Verified absence of reconnects which disabling atomic connects.

@bsenthilr bsenthilr changed the title Make decorators optional and disable atomic connections by default Make GRPC transport decorators configurable & disable atomic decorator by default Feb 18, 2025
Copy link
Collaborator

@kta-intel kta-intel left a comment

Choose a reason for hiding this comment

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

Thanks Senthil, LGTM

Just wanted to confirm (and for historical context, in case anyone comes across this thread), that a user should still be able to use atomic connections via the plan.yaml?

network:
  settings:
    enable_atomic_connections: True

@bsenthilr
Copy link
Collaborator Author

bsenthilr commented Feb 18, 2025

Just wanted to confirm (and for historical context, in case anyone comes across this thread), that a user should still be able to use atomic connections via the plan.yaml?

network:
  settings:
    enable_atomic_connections: True

good call, fixed @kta-intel . There is certainly gap on lacking capability to set on desired collabs. But this is a good start. Defaulted to false under network settings for now.

Log connections on info level for visibility

Signed-off-by: Balakrishnan, Senthil <[email protected]>
Signed-off-by: Balakrishnan, Senthil <[email protected]>
… set this independent on individual collabs

Signed-off-by: Balakrishnan, Senthil <[email protected]>
Copy link
Collaborator

@kta-intel kta-intel left a comment

Choose a reason for hiding this comment

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

good call, fixed @kta-intel . There is certainly gap on lacking capability to set on desired collabs. But this is a good start. Defaulted to false under network settings for now.

Thanks for checking. Considering that the original atomic connection didn't have that functionality either, this is already a good addition

Copy link
Member

@MasterSkepticista MasterSkepticista left a comment

Choose a reason for hiding this comment

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

Minor change, good to go.

@bsenthilr
Copy link
Collaborator Author

@kagrawa2 is going to add some updates for collab reconnection resiliency under reconnect decorator

@bsenthilr
Copy link
Collaborator Author

Minor change, good to go.

better to do add rate limiting log support on next PR. That would be handy for several situations.
one reference (lets look for the most adopted library and plumb it in):
from log_rate_limit import StreamRateLimitFilter logger.addFilter(StreamRateLimitFilter(period_sec=30))

Copy link
Collaborator

@ishaileshpant ishaileshpant left a comment

Choose a reason for hiding this comment

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

apart from updates to documents LGTM

@bsenthilr bsenthilr merged commit ee28d8c into securefederatedai:develop Feb 19, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants