-
Notifications
You must be signed in to change notification settings - Fork 9
configure mem pool buckets rotation #261
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
base: main
Are you sure you want to change the base?
configure mem pool buckets rotation #261
Conversation
18c714e to
16d302f
Compare
request/pool.go
Outdated
| rp.semaphore.Release(1) | ||
| }, | ||
| Epoch: time.Second, | ||
| Epoch: time.Duration(rand.Intn(50))*time.Millisecond + rp.options.FirstStrikeThreshold/10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the mean of the epoch is longer than FirstStrikeThreshold / 10; in fact it is FirstStrikeThreshold / 10 +25ms. I don't think this is the right approach.
Draw a random number d whose range is [-D,D]. Then add the "dither" d when you compute if a first strike had happened, e.g. in here:
func (ps *PendingStore) checkFirstStrike(now time.Time)
if now.Sub(bucket.lastTimestamp) <= ps.FirstStrikeThreshold { <<<
continue
}
by adding d to the ps.FirstStrikeThreshold.
In any event, this is not expected to help much if d is much smaller than the batching interval. If the first TXs forwarded to the primary (by the secondary who strikes first) don't return in a batch by the time the next secondary batcher strikes, they will be forwarded anyway....
It seems to me a good choice for D would be around 2*BatchInterval or 3*BatchInterval.... what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
0928df9 to
4c92ff7
Compare
| Logger: rp.logger, | ||
| SecondStrikeThreshold: rp.options.SecondStrikeThreshold, | ||
| FirstStrikeThreshold: rp.options.FirstStrikeThreshold, | ||
| FirstStrikeThreshold: rp.options.FirstStrikeThreshold + rp.addRandomnessToFirstStrike(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is adding randomness only once, when the pool is created. Wouldn't it make more sense to do it per bucket, or would that be too much? what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be too much
| rp.pending.Start() | ||
| } | ||
|
|
||
| func (rp *Pool) addRandomnessToFirstStrike() time.Duration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets make sure that in the worse case the first strike is still > BatchTimeout. IE compare the BatchTimeout with the FirstStrikeThreshold. It seems to me the FirstStrikeThreshold should be at least 5*BatchTimeout or so...
Even without randomization, if FirstStrikeThreshold < BatchTimeout + network-delay we'll get constant forwarding. So some sanity checks are required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add checks
Currently the batch timeout is 500ms and we are running tests with first strike of 2s
4c92ff7 to
f00a6a5
Compare
7523481 to
656138d
Compare
4a18503 to
2686135
Compare
1493556 to
743f9b4
Compare
4ce27cf to
534c31f
Compare
Signed-off-by: Hagar Meir <[email protected]>
Signed-off-by: Hagar Meir <[email protected]>
Signed-off-by: Hagar Meir <[email protected]>
Signed-off-by: Hagar Meir <[email protected]>
534c31f to
c0eba11
Compare
|
We park this one until we return and implement a robust comprehensive method of dealing with Router failure. We can then evaluate this technique here relative or in combination with the comprehensive approach. |
#269