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

Client stats #63

Closed
wants to merge 10 commits into from
Closed

Client stats #63

wants to merge 10 commits into from

Conversation

mkabischev
Copy link

Hi, this PR adds possibility to get information about number of active & idle connections.

@superstas
Copy link

Hello @bradfitz, could you please review this PR.
The changes here would help us a lot.

Thank you!

Copy link
Owner

@bradfitz bradfitz left a comment

Choose a reason for hiding this comment

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

Please also squash this into a single commit.

@@ -129,6 +129,12 @@ func NewFromSelector(ss ServerSelector) *Client {
return &Client{selector: ss}
}

// Stats contains statistic about connections being used by client.
Copy link
Owner

Choose a reason for hiding this comment

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

statistics (plural)

// Stats contains statistic about connections being used by client.
type Stats struct {
ActiveConns int
IdleConns int
Copy link
Owner

Choose a reason for hiding this comment

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

document these two also.

@@ -144,9 +150,12 @@ type Client struct {
// be set to a number higher than your peak parallel requests.
MaxIdleConns int

// number of currently used connections
Copy link
Owner

Choose a reason for hiding this comment

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

add docs that this must be accessed atomically

@@ -465,6 +477,21 @@ func (c *Client) GetMulti(keys []string) (map[string]*Item, error) {
return m, err
}

// Stats returns current statistic
Copy link
Owner

Choose a reason for hiding this comment

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

the current statistics. (with trailing period)

selector ServerSelector

lk sync.Mutex
lk sync.RWMutex
Copy link
Owner

Choose a reason for hiding this comment

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

doesn't seem worth it.

Choose a reason for hiding this comment

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

maybe more straightforward to just make idleConns an atomic int as well and inc/dec when freeconn changes?

mostynb and others added 6 commits June 27, 2018 23:29
code.google.com is in archive/maintenance mode, and redirects the original
URL to:
https://cloud.google.com/appengine/docs/standard/go/memcache/reference

Let's just use a godoc URL instead.
Replace code.google.com URL with godoc.org URL
* Use ReadFull instead of ioutil.ReadAll to read objects

ioutil.ReadAll uses a bytes.Buffer, which allocates memory by doubling.
Since we know exactly how muh data we expect to get, we can allocate it
in advance. This reduces the total amount of allocation, and ensures
that the slice stored in the item won't have excess capacity (which can
affect memory usage if the item is held for a long time, for instance in
a secondary cache).
This is mentioned in the Expiration field of the Item struct (no
expiration time), but not in the notes about the Touch function.
- [go modules](https://github.com/golang/go/wiki/Modules) will be enabled in 1.13.
- after merge it would be nice if you would tag this repo: e.g. `v1.0.0`
  - this will allow consumers something more consistent and readable than `v0.0.0-20181229203832-0af3f3b09a0a`.
@dragonsinth
Copy link

Can I vote on this? Or offer to address the feedback in this PR? We also need this info for stats / monitoring.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@dragonsinth
Copy link

@mkabischev are you opening a new one rebased instead of merged?

@dragonsinth dragonsinth mentioned this pull request Sep 16, 2020
@dragonsinth
Copy link

Opened a version as #124

mikelolasagasti added a commit to mikelolasagasti/gomemcache that referenced this pull request Mar 14, 2024
Return ss.SetServers()'s error to make debugging easier.

Updates bradfitz#83

Closes bradfitz#63

Signed-off-by: Mikel Olasagasti Uranga <[email protected]>
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.

9 participants