[services] Tunnelbroker - AMQP message payload string conversion to use size and remove double `getPayload()` call
ClosedPublic
Actions

Authored by • max on Aug 1 2022, 12:36 PM.

Details

Reviewers

• karol
tomek

Commits

rCOMMaa2b2108afbf: [services] Tunnelbroker - AMQP message payload string conversion to use size…

Summary

This is a small improvement and possibly a bug fix. We should use a message body size when constructing a string from an AMQP message payload instead of using body-only.

As a small follow-up presented in this diff: fix message->getPayload() double call and use the local instance instead. It's a super small change that's why merged it into a single one but a small diff to save a reviewer's time.

Linear task: ENG-1495

Test Plan

Successfully built the server using yarn run-tunnelbroker-service-in-sandbox command. Run tests using yarn run-unit-tests

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Aug 1 2022, 12:36 PM

• max held this revision as a draft.

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptAug 1 2022, 12:36 PM

• max published this revision for review.Aug 1 2022, 12:42 PM

• max retitled this revision from [services] Tunnelbroker - AMQP message payload string conversion to use size to [services] Tunnelbroker - AMQP message payload string conversion to use size and remove double `getPayload()` call.

• max edited the summary of this revision. (Show Details)

• max added reviewers: • karol, tomek.

Harbormaster completed remote builds in B11022: Diff 15163.Aug 1 2022, 12:49 PM

tomek accepted this revision.Aug 2 2022, 9:55 AM

tomek added inline comments.

services/tunnelbroker/src/Amqp/AmqpManager.cpp
66 ↗	(On Diff #15163)	I guess it shouldn't hurt to use this constructor, but what is the possible benefit of this approach? This is a small improvement and possibly a bug fix. We should use a message body size when constructing a string from an AMQP message payload instead of using body-only. Why should we do that? Does the documentation suggest it?

This revision is now accepted and ready to land.Aug 2 2022, 9:55 AM

• max marked an inline comment as done.Aug 2 2022, 11:32 AM

• max added inline comments.

services/tunnelbroker/src/Amqp/AmqpManager.cpp
66 ↗	(On Diff #15163)	I guess it shouldn't hurt to use this constructor, but what is the possible benefit of this approach? This is a small improvement and possibly a bug fix. We should use a message body size when constructing a string from an AMQP message payload instead of using body-only. Why should we do that? Does the documentation suggest it? There is not so much documentation in AMQP-CPP, but I've caught that some of the tests failed due to the difference in the payload. Investigation shows that the `string payload(message.body());` can contain some of the bytes not from the payload itself. The investigation further I've found that some of the code that use AMQP-CPP (Clickhouse database) for example, use the `bodySize()` from AMQP `Envelope` in a string constructor. That fixes tests and payload. So I've made this fix in our codebase too.