Page MenuHomePhabricator

[services] Tunnelbroker - Database public functions for messages.
ClosedPublic

Authored by max on Feb 6 2022, 5:43 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jun 30, 8:42 PM
Unknown Object (File)
Sun, Jun 30, 5:16 PM
Unknown Object (File)
Sun, Jun 30, 5:16 PM
Unknown Object (File)
Sun, Jun 30, 5:16 PM
Unknown Object (File)
Sun, Jun 30, 5:16 PM
Unknown Object (File)
Sun, Jun 30, 5:15 PM
Unknown Object (File)
Sun, Jun 30, 5:08 PM
Unknown Object (File)
Sun, Jun 30, 1:13 AM

Details

Summary

DynamoDB database public functions and on-start check integration for persisting messages that pass in tunnelbroker.

Related linear task: ENG-670

*Dependencies:*

  • Depends on D3114 to provide MessageItem entities.
Test Plan

Check that putMessageItem, findMessageItem, removeMessageItem returns expected results.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

max added reviewers: karol, tomek.
tomek requested changes to this revision.Feb 7 2022, 2:27 AM
tomek added inline comments.
services/tunnelbroker/docker-server/contents/server/src/Database/DatabaseManager.cpp
181–184 ↗(On Diff #9326)

In putSessionItem and in blob service we use a different approach

request.AddItem(
      DeviceSessionItem::FIELD_CHECKPOINT_TIME,
      Aws::DynamoDB::Model::AttributeValue(
          std::to_string(item.getCheckpointTime()))

We call AttributeValue instead of calling SetN method. Could you clarify which approach is correct and make sure to be consistent?

This revision now requires changes to proceed.Feb 7 2022, 2:27 AM
max added inline comments.
services/tunnelbroker/docker-server/contents/server/src/Database/DatabaseManager.cpp
181–184 ↗(On Diff #9326)

In putSessionItem and in blob service we use a different approach

request.AddItem(
      DeviceSessionItem::FIELD_CHECKPOINT_TIME,
      Aws::DynamoDB::Model::AttributeValue(
          std::to_string(item.getCheckpointTime()))

We call AttributeValue instead of calling SetN method. Could you clarify which approach is correct and make sure to be consistent?

If we call AttributeValue the DynamoDB saves the value as a string (it's same as calling SetS). If we need to save the value as an integer we should call SetN.

max marked an inline comment as done.
tomek requested changes to this revision.Feb 9 2022, 3:44 AM
tomek added inline comments.
services/tunnelbroker/docker-server/contents/server/src/Database/DatabaseManager.cpp
181–184 ↗(On Diff #9326)

Is it safe to save uint64_t by using this method? In the documentation you linked I can't see that this method has this (or long long) variant. So it looks like we're going to cast the value to int - we should definitely fix that.

This revision now requires changes to proceed.Feb 9 2022, 3:44 AM
services/tunnelbroker/docker-server/contents/server/src/Database/DatabaseManager.cpp
181–184 ↗(On Diff #9326)

What's wrong with storing it in the dynamo as a string?

I think we should always work with strings when dealing with dynamo DB, even GetN returns a string.

The first thing is what Tomek mentioned about numeric data types - doesn't look safe.
Another thing is: is there a method that would return a number when fetching from the database?

max marked 2 inline comments as done.

Rebase and remove SetN.

max added inline comments.
services/tunnelbroker/docker-server/contents/server/src/Database/DatabaseManager.cpp
181–184 ↗(On Diff #9326)

Is it safe to save uint64_t by using this method? In the documentation you linked I can't see that this method has this (or long long) variant. So it looks like we're going to cast the value to int - we should definitely fix that.

What's wrong with storing it in the dynamo as a string?

I think we should always work with strings when dealing with dynamo DB, even GetN returns a string.

The first thing is what Tomek mentioned about numeric data types - doesn't look safe.
Another thing is: is there a method that would return a number when fetching from the database?

I thought that the point why we have SetS and SetN while the function returns and gets string: it's because of how DynamoDB stores it. If we call SetN it will cast and store the value as a 64-bit integer.

But when I dig into it I've found that SetN is a simple helper to convert the numbers to the string... And this comment confirms this too.

That's why there is no any point to use SetN anywhere.
I've switched to the default AttributeValue which is SetS inside....

This revision is now accepted and ready to land.Feb 14 2022, 5:40 AM
This revision now requires review to proceed.Feb 14 2022, 5:40 AM
ashoat added a reviewer: jim.
This revision is now accepted and ready to land.Feb 14 2022, 8:49 PM
This revision was landed with ongoing or failed builds.Feb 15 2022, 12:23 PM
This revision was automatically updated to reflect the committed changes.