-
Notifications
You must be signed in to change notification settings - Fork 247
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
feat_: hash based query for outgoing messages. #5217
Conversation
Jenkins BuildsClick to see older builds (106)
|
27167d2
to
e719178
Compare
79399ce
to
6b56b45
Compare
messageHashes[i] = pb.ToMessageHash(hash.Bytes()) | ||
} | ||
|
||
result, err := w.node.Store().QueryByHash(ctx, messageHashes, opts...) |
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.
We should consider store Query limits of how many hashes can be queried at once and probably batch these requests in parallel to multiple store nodes.
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.
Yeah, added limit, batch seems overkill since there is not many messages in a few seconds.
d305d7f
to
9b924a4
Compare
pubsubMessageIds := make([][]gethcommon.Hash, 0, len(w.sendMsgIDs)) | ||
for pubsubTopic, subMsgs := range w.sendMsgIDs { | ||
var queryMsgIds []gethcommon.Hash | ||
for msgID, sendTime := range subMsgs { |
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.
just to make sure, here you are taking max 20 random messages from the one sent to check on store? maybe we should use a sorted map (or any sorted struct)
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.
For each pubsub topic, it will choose 20 random outgoing messages for the check on store.
After a quick search, there is no sorted map built in Go, adding extra logic for this feature seems overkill consider the frequency of outgoing messages. @cammellos
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.
Overall LGTM. FYI, just saw sendDataSync invoked transport.TrackMany
, so the packet sent via MVDS should also be monitored, it will retry with handleEnvelopeFailure
until it hit maxAttempts
. It would be cool if you can add a test @kaichaosun
d78064f
to
d1a2e5f
Compare
Wondering if there is an overlap between this and https://github.com/status-im/status-go/pull/5281/files. |
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.
LGTM or than few minor comments.
Would love to see the effectiveness and how much additional bandwidth is consumed during dogfooding.
This PR is for outgoing messages, #5281 is for incoming messages if I'm not mistaken. @chaitanyaprem |
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.e if a message is acknowledged e2e, it should be removed
if the message is acknowledged by the other peer, we should stop resending, we do that for datasync messages already, but we want to do the same in kaichao's PR
It seems we haven't reached this goal with this PR yet? watchExpiredMessages will check the raw_messages
table every second and there's still a chance it will resend the sent message? @kaichaosun
@qfrank for messages acked through mvds, it's deleted in the query queue, so won't be marked as expired. Not sure if there's other ack or not other than mvds. |
@qfrank mentioned there is an edge case that raw message resend could be triggered just before the message marked as sent, likely happen in a few milliseconds. This is possible because the coordination depends on the database table |
Hi @kaichaosun , just had a DM with @cammellos , we can deal with it at a later time, worst case we send a message twice, but we won't process the same message twice on the receiver side according to this, so just wasteful. Thank you for your PR! |
@kaichaosun has this been tested in the clients? I think at least running e2e on mobile should have been done before merging it, unless the feature is disabled, but I don't see any flag |
I have tested it with Should we halt the changes for more QAs? @cammellos |
@kaichaosun it's probably ok, maybe next time ping QA so they can run e2e tests on the build |
For outgoing messages, only mark it as it sent after successfully found in store node, the messages are not found will be marked as expired and resend.
Important changes:
EventEnvelopeSent
, messages missed will triggerEventEnvelopeExpired
Relates to #5234