Blockwise: Use hash instead of token for cache #554
Conversation
WalkthroughThe recent changes involve updating various functions and methods across multiple files to utilize a Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
mpenate-ellenbytech
force-pushed
the
blockwise-hash
branch
from
bb2fd9b
to
7098e70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- dtls/client.go (1 hunks)
- dtls/server/server.go (1 hunks)
- message/option.go (4 hunks)
- net/blockwise/blockwise.go (21 hunks)
- net/blockwise/blockwise_test.go (2 hunks)
- net/client/client.go (1 hunks)
- net/observation/handler.go (1 hunks)
- tcp/client.go (2 hunks)
- tcp/server/server.go (1 hunks)
- udp/client.go (2 hunks)
- udp/server/server.go (1 hunks)
Additional comments not posted (21)
tcp/client.go (1)
81-82: Update the parameter type tohash uint64in theGetObservationRequestfunction call.This change aligns with the objective of using a hash instead of a token for cache.
udp/client.go (1)
80-81: Update the parameter type tohash uint64in theGetObservationRequestfunction call.This change aligns with the objective of using a hash instead of a token for cache.
dtls/client.go (1)
93-94: Update the parameter type tohash uint64in theGetObservationRequestfunction call.This change aligns with the objective of using a hash instead of a token for cache.
tcp/server/server.go (1)
196-196: Update the parameter type tohash uint64in theGetObservationRequestfunction call.This change aligns with the objective of using a hash instead of a token for cache.
dtls/server/server.go (1)
201-202: Update the parameter type tohash uint64in theGetObservationRequestfunction call.This change aligns with the objective of using a hash instead of a token for cache.
net/client/client.go (1)
108-109: Update the parameter type tohash uint64in theGetObservationRequestfunction.This change aligns with the objective of using a hash instead of a token for cache.
net/observation/handler.go (1)
111-112: Update method signature to use hash instead of token.The change to use
hash uint64instead oftoken message.Tokenin theGetObservationRequestmethod aligns with the PR objective and appears correct.udp/server/server.go (1)
281-286: Update method signature to use hash instead of token.The change to use
hash uint64instead oftoken message.Tokenin thegetOrCreateConnmethod aligns with the PR objective and appears correct.message/option.go (2)
50-50: Add new optionRequest-Tag.The addition of the
Request-Tagoption withOptionID 292aligns with the PR objective and appears correct.
77-77: AddRequestTagtooptionIDToStringmap.The addition of the
RequestTagoption to theoptionIDToStringmap ensures proper string representation and aligns with the PR objective.Also applies to: 101-101
net/blockwise/blockwise_test.go (1)
58-60: The addition of theRemoteAddrmethod looks good and provides a hardcoded IP address for testing purposes.net/blockwise/blockwise.go (10)
8-8: Import ofhash/crc64is necessary for generating hash values.
146-146: The function signature update to usehash uint64instead oftoken message.Tokenaligns with the new caching mechanism.
168-171: The function signature and default value update to usehash uint64instead oftoken message.Tokenis consistent with the new caching mechanism.
222-223: The addition of thematchableHashvariable to store the hash value generated from the message options, remote address, and code is necessary for the new caching mechanism.
291-293: The update of theWriteMessagefunction to usematchableHashis necessary for the new caching mechanism.
342-343: The addition of thegetSendingMessageCodefunction to retrieve the code of the sending message using the hash value is necessary for the new caching mechanism.
357-358: The update of theHandlefunction to usematchableHashis necessary for the new caching mechanism.
Line range hint
389-421: The update of thehandleReceivedMessagefunction to userx_hashis necessary for the new caching mechanism.
Line range hint
540-563: The update of thestartSendingMessagefunction to userx_hashis necessary for the new caching mechanism.
756-779: The addition of thegenerateMatchableHashfunction to generate a hash value from the message options, remote address, and code is necessary for the new caching mechanism.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- dtls/client.go (1 hunks)
- dtls/server/server.go (1 hunks)
- message/option.go (4 hunks)
- net/blockwise/blockwise.go (21 hunks)
- net/blockwise/blockwise_test.go (2 hunks)
- net/client/client.go (1 hunks)
- net/observation/handler.go (1 hunks)
- tcp/client.go (2 hunks)
- tcp/server/server.go (1 hunks)
- udp/client.go (2 hunks)
- udp/server/server.go (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- dtls/client.go
- dtls/server/server.go
- message/option.go
- net/blockwise/blockwise_test.go
- net/client/client.go
- net/observation/handler.go
- tcp/client.go
- tcp/server/server.go
- udp/client.go
- udp/server/server.go
Additional comments not posted (10)
net/blockwise/blockwise.go (10)
8-8: The addition of thecrc64import is appropriate for the new hash generation functionality.
146-146: The function signature update to usehash uint64instead oftoken message.Tokenaligns with the new hash-based cache identification approach.
168-171: The update to theNewfunction to acceptgetSentRequestFromOutsidewithhash uint64is consistent with the new hash-based approach.
222-223: The addition of thematchableHashgeneration in theDofunction is crucial for the new hash-based cache mechanism.
291-293: The addition of thematchableHashgeneration in theWriteMessagefunction is necessary for the new hash-based cache mechanism.
342-343: The addition of thegetSendingMessageCodefunction to retrieve the message code using the hash is essential for the new hash-based approach.
357-358: The addition of thematchableHashgeneration in theHandlefunction is necessary for the new hash-based cache mechanism.
514-515: The addition of thematchableHashgeneration in thecontinueSendingMessagefunction is necessary for the new hash-based cache mechanism.
Line range hint
540-563: The update to thestartSendingMessagefunction to use therx_hashis consistent with the new hash-based approach.
756-777: The addition of thegenerateMatchableHashfunction is a critical component of the new hash-based cache mechanism.
Credit to @zworks-okada for initial work regarding rx transfers. Expanded to include tx. Closes plgd-dev#512
mpenate-ellenbytech
force-pushed
the
blockwise-hash
branch
from
7098e70
to
798345f
Compare
|
Fixed linting issues. I should add that I am not a golang programmer, and addressing test failures will be difficult without any help. I can ask a colleague for some of their input but direct help would be appreciated. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- dtls/client.go (1 hunks)
- dtls/server/server.go (1 hunks)
- message/option.go (4 hunks)
- net/blockwise/blockwise.go (21 hunks)
- net/blockwise/blockwise_test.go (2 hunks)
- net/client/client.go (1 hunks)
- net/observation/handler.go (1 hunks)
- tcp/client.go (2 hunks)
- tcp/server/server.go (1 hunks)
- udp/client.go (2 hunks)
- udp/server/server.go (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- dtls/client.go
- dtls/server/server.go
- message/option.go
- net/blockwise/blockwise_test.go
- net/client/client.go
- net/observation/handler.go
- tcp/client.go
- tcp/server/server.go
- udp/client.go
- udp/server/server.go
Additional comments not posted (10)
net/blockwise/blockwise.go (10)
8-8: Import ofcrc64looks good.
137-138: Addition ofRemoteAddr()method to theClientinterface looks good.
146-146: Update to useuint64for caching inBlockWisestruct looks good.
168-171: Update to theNewconstructor function to acceptfunc(hash uint64) (*pool.Message, bool)looks good.
222-227: Update to theDomethod to generate and use a matchable hash looks good.
291-293: Update to theWriteMessagemethod to generate and use a matchable hash looks good.
342-343: Introduction of thegetSendingMessageCodemethod looks good.
357-360: Update to theHandlemethod to generate and use a matchable hash looks good.
Line range hint
389-421: Update to thehandleReceivedMessagemethod to accept arxHash uint64parameter looks good.
756-774: Introduction of thegenerateMatchableHashfunction looks good.
| @@ -504,7 +511,8 @@ func (b *BlockWise[C]) continueSendingMessage(w *responsewriter.ResponseWriter[C | |||
| } | |||
| var sendMessage *pool.Message | |||
| var more bool | |||
| b.sendingMessagesCache.LoadWithFunc(r.Token().Hash(), func(value *cache.Element[*pool.Message]) *cache.Element[*pool.Message] { | |||
| matchableHash := generateMatchableHash(r.Options(), w.Conn().RemoteAddr(), r.Code()) | |||
There was a problem hiding this comment.
@mpenate-ellenbytech Thank you for contribution :)
This is an issue: the hash for the response will not match the request hash because r.Code() is different, and r.Options(), such as the URI, are not part of the response. Therefore, you cannot pair it correctly for block1.
For sending a big payload (Block1):
You can use RemoteAddress and MessageID, which need to be registered for each new request of type Confirmation/NonConfirmation and response. If MessageID is not supported, then only the remote address can be used to verify block1.
For receiving a big payload (Block2):
When you want to locate the sent request, this will not work because the hash will again be different. Therefore, you need to pair it with the request using MessageID. However, you need to set a new MessageID for each exchange (request and response).
BTW: For blockwise transfer via TCP, the token must be used as implemented now.
Therefore, create blockwiseTCP.go (original code) and blockwiseUDP.go, where blockwiseUDP.go will utilize MessageIDs.
| for _, opt := range options { | ||
| switch opt.ID { | ||
| // Skip Blockwise Options and NoCacheKey Options | ||
| case message.Block1, message.Block2, message.Size1, message.Size2, message.RequestTag: |
There was a problem hiding this comment.
I'm not sure what this hash is used for, but you may want to exclude RequestTag here.
Yes, the text says it is not part of the matchable set, but later on it says (roughly) that even if two requests are matchable, if they have different request tags, they should not be matched.
When I wrote RFC9175, we had the choice of wording while not changing the actual mechanism: We went for easier for those implementing the client side (where Request-Tag is excluded for the definition of being matchable, and then if two requests are matchable even if you don't want them to be, you add a Request-Tag and there is an extra rule on it for the server), or easier for those implementing the server side (when Request-Tag would not have been special, but in all the client sections we would have said "if the request are matchable except for possibly differing Request-Tag values"). We went for the former -- I'm not sure it was the right decision, especially if generateMatchableHash is used on the server side. (I can't tell easily without having more context of go-coap, was just pointed to this issue by a colleague).
There was a problem hiding this comment.
Also, the NoCacheKey options could more generally be recognzied from their option properties (lower bits of the number) rather than enumerating them (which will not be exhaustive).
Credit to @zworks-okada for initial work regarding rx transfers. Expanded to include tx.
Closes #512
Needs more thorough testing than what I was able to do. Willing to help address concerns.
Summary by CodeRabbit
New Features
Request-Tagoption for enhanced request handling.Refactor
hash uint64instead ofmessage.Tokenfor observation requests, improving consistency and performance.ClientandServerstructs for better data handling.Improvements
RemoteAddr()method to client interfaces for better address handling.These changes streamline observation request processing and enhance the overall efficiency of the application.