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

Make PollingBased waiter more flexible #3556

Merged
merged 5 commits into from
Aug 20, 2024
Merged

Make PollingBased waiter more flexible #3556

merged 5 commits into from
Aug 20, 2024

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Aug 13, 2024 •
edited
Loading

Networks with short block time can't use 1s as a default poll interval, it's too harsh for them. Ref. nspcc-dev/neofs-node#2864.

@AnnaShaleva AnnaShaleva added this to the v0.106.4 milestone Aug 13, 2024
@AnnaShaleva AnnaShaleva added I3 Minimal impact rpc RPC server and client U3 Regular S4 Routine labels Aug 13, 2024
@AnnaShaleva
Copy link
Member Author

OK, tests are failing and we need to adjust Actor correspondingly.

@AnnaShaleva AnnaShaleva marked this pull request as draft August 13, 2024 15:25
@AnnaShaleva
Copy link
Member Author

@roman-khimov, regarding Actor integration I consider two options:

  1. One more Actor constructor that accepts PollConfig, but we have a lot of othre Actor constructor and this config is not related to the Actor directly.
  2. (*Actor).WithPollConfig(cfg PollConfig) modifier which seems a bit more preferable to me, because it's a bit easier to use. At the same time it's different from Actor's design.

Which one do you prefer? Or may be there's another option.

@AnnaShaleva AnnaShaleva marked this pull request as ready for review August 13, 2024 16:21
@codecov Codecov
Copy link

codecov bot commented Aug 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 65.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 86.05%. Comparing base (7766168) to head (4255566).
Report is 6 commits behind head on master.

Files Patch % Lines
pkg/rpcclient/waiter/waiter.go 63.15% 7 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #3556   +/-   ##
=======================================
  Coverage   86.05%   86.05%           
=======================================
  Files         330      330           
  Lines       38692    38697    +5     
=======================================
+ Hits        33297    33302    +5     
  Misses       3849     3849           
  Partials     1546     1546

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

roman-khimov
roman-khimov reviewed Aug 13, 2024
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

How about extending actor.Options with waiter.PollConfig?

pkg/services/rpcsrv/subscription_test.go Outdated Show resolved Hide resolved
pkg/rpcclient/waiter/waiter.go Outdated Show resolved Hide resolved
pkg/rpcclient/waiter/waiter.go Outdated Show resolved Hide resolved
@AnnaShaleva
Copy link
Member Author

How about extending actor.Options with waiter.PollConfig?

I thought about it, but it's not really Acttor's configuration. Will add if you're OK with it.

@roman-khimov
Copy link
Member

I'm also thinking of waiter.Config in case we want to extend it in future.

@AnnaShaleva
Copy link
Member Author

AnnaShaleva commented Aug 14, 2024 •
edited
Loading

I'm also thinking of waiter.Config in case we want to extend it in future.

I also thought about it, but WSBased does not use any configurable options now, thus I created just a PollConfig. Let's extend it while we're here, it'll be useful.

@AnnaShaleva AnnaShaleva force-pushed the adjust-waiter branch 3 times, most recently from 43dba56 to 842fb8b Compare August 14, 2024 08:48
@AnnaShaleva
Copy link
Member Author

Linter job is failing, but it's just because golangci-lint version was updated, will be fixed in a separate PR.

@AnnaShaleva
Copy link
Member Author

AnnaShaleva commented Aug 19, 2024 •
edited
Loading

@roman-khimov, this one is ready for review. It's not needed for NeoFS anymore, but still it would be nice to have these values customizable.

@roman-khimov
Copy link
Member

NeoFS is likely to still use it in some way.

roman-khimov
roman-khimov reviewed Aug 19, 2024
View reviewed changes
pkg/rpcclient/waiter/waiter.go Outdated
// newCustomPollingBased is an internal constructor of PollingBased waiter that sets
// default configuration values if needed.
func newCustomPollingBased(waiter RPCPollingBased, v *result.Version, config PollConfig) *PollingBased {
if config.PollInterval == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

<=?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

pkg/rpcclient/waiter/waiter.go Outdated
if config.PollInterval == 0 {
config.PollInterval = time.Millisecond * time.Duration(v.Protocol.MillisecondsPerBlock) / 2
}
if config.RetryCount == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

<=?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

pkg/rpcclient/waiter/waiter.go Show resolved Hide resolved
pkg/rpcclient/waiter/waiter.go Show resolved Hide resolved
@AnnaShaleva
rpcclient: remove default PolingBased poll interval
3e3991c 
pollTime is never 0 since MillisecondsPerBlock protocol configuration value
is present in `getversion` RPC response since 0.97.3 release. We don't have such
old RPC servers in the network anymore, thus this fallback code may be
safely removed.

Signed-off-by: Anna Shaleva <[email protected]>
@AnnaShaleva
rpcclient: allow to tune PollingBased waiter
027d726 
Some clients need more flexible awaiting options (e.g. for short-blocks
networks). The default behaviour is not changed, all exported APIs are
compatible. Ref. nspcc-dev/neofs-node#2864.

Signed-off-by: Anna Shaleva <[email protected]>
@AnnaShaleva
rpcsrv: add test for Waiter constructor
afbb51e 
Ensure that WSClient-based Actor is able to create EventBased waiter.

Signed-off-by: Anna Shaleva <[email protected]>
@AnnaShaleva
rpcclient: integrate customizable Waiter with Actor
92c6361 
Signed-off-by: Anna Shaleva <[email protected]>
@AnnaShaleva
rpcclient: add Waiter.Config
4255566 
Include Waiter.PollConfig into Waiter.Config and use extended Waiter
configuration where needed.

Signed-off-by: Anna Shaleva <[email protected]>
@roman-khimov roman-khimov merged commit dfd4566 into master Aug 20, 2024
20 of 21 checks passed
@roman-khimov roman-khimov deleted the adjust-waiter branch August 20, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

@AliceInHunterland AliceInHunterland Awaiting requested review from AliceInHunterland

Assignees
No one assigned
Labels
I3 Minimal impact rpc RPC server and client S4 Routine U3 Regular
Projects
None yet
Milestone
v0.107.0
Development

Successfully merging this pull request may close these issues.
2 participants
@AnnaShaleva @roman-khimov