swarm/network: measure time of messages in priority queue #19250
Conversation
swarm/network/stream/peer.go
Outdated
| @@ -154,7 +154,7 @@ func (p *Peer) Deliver(ctx context.Context, chunk storage.Chunk, priority uint8, | |||
| } | |||
|
|
|||
| ctx = context.WithValue(ctx, "stream_send_tag", nil) | |||
| return p.SendPriority(ctx, msg, priority) | |||
| return p.Send(ctx, msg) | |||
There was a problem hiding this comment.
why change it here?
either leave it or change everywhere by temporarily redefining sendPriority
There was a problem hiding this comment.
I change it so that we can use tracing to debug this problem. SendPriority doesn't support traces.
Furthermore I posted measurements of the priority queue in the chat yesterday, which show that messages could stay in the priority queue for tens-of-seconds, which is really bad:
Bottom line, I try to reduce the number of packages that behave weird in order to understand the timeout issue.
Smoke tests have been passing for the past 16 hours consistently with this PR, which means that using Send here is just fine at least according to our current test suite.
@zelig do we have any actual test that show the we must use a priority queue for send RPC calls? Generally all our RPC handlers are very fast and the handleIncoming loop is really tight, so the priority queue doesn't make much sense to me.
Note that when the messages stayed for that long in the queue, there were no error messages logged that suggest queue contention.
There was a problem hiding this comment.
good job @nonsense could you try setting the syncer streamer priority from top to mid to understand what was happening
nonsense
force-pushed
the
pq-measurements
branch
4 times, most recently
from
65adf5a to
3b1b81b
Compare
nonsense
force-pushed
the
pq-measurements
branch
from
807bd39 to
d972357
Compare
| err := trackChunks(randomBytes[:]) | ||
| if err != nil { | ||
| log.Error(err.Error()) | ||
| // trigger debug functionality on randomBytes |
There was a problem hiding this comment.
do we need to trigger this even if err is nil?
There was a problem hiding this comment.
yes, so that we compare success runs vs failed runs.
This PR is:
inputSeedflag to the smoke tests, so that we can reproduce an exact upload to a given Swarm deployment.trackChunksboth on successful and on failed smoke test runs, so that we can compare between them.DebugandTracelogs in some packages.f()(generally ap2p.Send()call) in our priority queue.streamAPI private.