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

Allow SampleRescaleThreshold to be configured from userspace #142

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

Conversation

narqo
Copy link

@narqo narqo commented Dec 3, 2015

Port fixes from #72 as it looks abandoned.

p.s. Still not sure it's the best possible solution, so ideas are welcome.

@mihasya
Copy link
Collaborator

mihasya commented Dec 3, 2015

Narqo, thanks for the updated PR. After re-reading the last PR and reading @rcrowley I think the main concern here is concurrent access. Using a mutex as in the original patch is the "obvious" solution. Richard was concerned about adding a mutex, presumably for performance and complexity reasons. However, looking at the code, it appears that the interval is cached on a per-sample basis in t1, so it's only ever called *once per sample per SampleRescaleThreshold. What do you think is a realistically low value for that? Maybe we should just go back to using the mutex since in computer terms it's not going to happen all that frequently at all.

Alternatively, we could just add a member on each Sample that caches the value and limit the need to sync to only initialization time at the cost of an extra time.Duration (int64).

One other point: this seems like the sort of thing that would cause "unexpected" behavior if changed during the runtime of an application, correct? Wondering if we should add a little code to protect from people doing this more than once.

@narqo
Copy link
Author

narqo commented Dec 4, 2015

Alternatively, we could just add a member on each Sample that caches the value and limit the need to sync to only initialization time at the cost of an extra time.Duration (int64)

I think this might be the simplest and quite safe solution we can do without compatibility break. Something like:

var SampleRescalThreshold = ...

type ExpDecaySample struct {
    ···
    rescaleThreshold time.Duration,
}

func NewExpDecaySample(reservoirSize int, alpha float64) Sample {
    ···
    s := &ExpDecaySample{
        rescaleThreshold: SampleRescalThreshold,
        ···
    }
}

I think to attach rescaleThreshold to the Registry instance would be a more proper solution. But currently, I don't think it's possible without breaking the API.

Wondering if we should add a little code to protect from people doing this more than once.

For sure.

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.

2 participants