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

MaxRequestsPerConn cluster param was added #1747

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

testisnullus
Copy link

@testisnullus testisnullus commented May 2, 2024

This PR allows a user to configure the maximum requests per connection parameter by a newly added MaxRequestsPerConn option.

Closes #1679
Functionality was adopted from similar PR from the ScyllaDB project: scylladb#113

@testisnullus
Copy link
Author

@martin-sucha Hello, will be glad to receive your review on this PR!

conn.go Outdated
@@ -257,7 +257,7 @@ func (s *Session) dialWithoutObserver(ctx context.Context, host *HostInfo, cfg *
errorHandler: errorHandler,
compressor: cfg.Compressor,
session: s,
streams: streams.New(cfg.ProtoVersion),
streams: s.streamIDGenerator(cfg.ProtoVersion),

Choose a reason for hiding this comment

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

what if we call it newStreamIDGenerator?
Also is it obligatory to pass config there if we access cfg from attached type?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, renamed and moved the function to the streams package.

defer h.mu.Unlock()
addr, _ := h.connectAddressLocked()
return net.JoinHostPort(addr.String(), strconv.Itoa(h.port))
h.mu.Lock()

Choose a reason for hiding this comment

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

I don't think it makes sense to format files which were not touched by the fix.

Copy link
Author

Choose a reason for hiding this comment

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

image

Reverted these changes, but it seems that they stayed inside this PR :\

@testisnullus testisnullus force-pushed the maxRequestsPerConn-cluster-param branch 2 times, most recently from 3c070c3 to ba0fac8 Compare May 23, 2024 08:48
@testisnullus testisnullus force-pushed the maxRequestsPerConn-cluster-param branch from ba0fac8 to 88405bb Compare May 23, 2024 08:52
Copy link

@taaraora taaraora left a comment

Choose a reason for hiding this comment

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

LGTM

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.

How to set pool size
2 participants