services/backup/docker-server/contents/server/src/DatabaseEntities/DatabaseEntitiesTools.h
5 ↗	(On Diff #9366)	We're not using this here. I can also see that the same happens in `blob` - we include items in `DatabaseEntitiesTools`. Is it for convenience, or we should avoid it?
services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.cpp
30 ↗	(On Diff #9366)	We're copying the vector, but I guess this might be a good idea - if we don't do that, someone could create this object, modify the vector variable used to create this and that would result in this object being changed.
55–57 ↗	(On Diff #9366)	Is it the recommended way of handling the bools?
services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.h
22 ↗	(On Diff #9366)	Why this isn't const?

This revision now requires changes to proceed.Feb 8 2022, 8:38 AM

• karol requested review of this revision.Feb 9 2022, 3:56 AM

• karol added inline comments.

services/backup/docker-server/contents/server/src/DatabaseEntities/DatabaseEntitiesTools.h
5 ↗	(On Diff #9366)	I think this has been mentioned before. The idea for including all entities in this file is we can include only the `DatabaseEntitiesTools.h` file and then be able to use all the entities with no need to include a header for every entity in every file where we want to use them. I think this is a nice shortcut and it doesn't hurt. If you insist on having every entity mentioned explicitly where it's used, we can refactor this later.
services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.cpp
30 ↗	(On Diff #9366)	We probably could also move the vector.
55–57 ↗	(On Diff #9366)	I'm not sure if recommended. Basically, we can skip the database part here as it's transparent to the problem The main problem is how to convert `string` to `bool`. There are a lot of ways of doing so, I find converting it to `int` the best but if you feel there is a more clear/elegant way of doing so, I'm open.
services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.h
22 ↗	(On Diff #9366)	This can be const but I think we should refactor all services https://linear.app/comm/issue/ENG-707/convert-table-names-to-const

tomek accepted this revision.Feb 9 2022, 9:19 AM

tomek added inline comments.

services/backup/docker-server/contents/server/src/DatabaseEntities/DatabaseEntitiesTools.h
5 ↗	(On Diff #9366)	It shouldn't matter too much in our case, so I guess we can prefer the convenience here and keep it included in this header.
services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.cpp
30 ↗	(On Diff #9366)	I'm usually slightly afraid of breaking the objects that someone gives us... but not sure, maybe it's a good idea.
55–57 ↗	(On Diff #9366)	I guess it's ok - it doesn't matter too much. Have you checked if in that case we could use `SetN` / `GetN`? They should write / read int, so maybe it would be a little safer?
services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.h
36 ↗	(On Diff #9366)	We should include `vector`

This revision is now accepted and ready to land.Feb 9 2022, 9:19 AM

Herald added a reviewer: ashoat. · View Herald TranscriptFeb 9 2022, 9:19 AM

Herald added a subscriber: • benschac. · View Herald Transcript

This revision now requires review to proceed.Feb 9 2022, 9:19 AM

ashoat accepted this revision.Feb 9 2022, 10:02 PM

ashoat added a reviewer: • jim.

ashoat added a subscriber: • jim.

ashoat added inline comments.

services/backup/docker-server/contents/server/src/DatabaseEntities/DatabaseEntitiesTools.h
5 ↗	(On Diff #9366)	@jimpo curious for your take here
services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.cpp
48 ↗	(On Diff #9366)	Is there a Linear task for this?

• karol edited the summary of this revision. (Show Details)Feb 10 2022, 6:32 AM

• karol added inline comments.Feb 10 2022, 7:07 AM

services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.cpp
30 ↗	(On Diff #9366)	Wasn't really sure what to do with this, so I created a task in the backlog as this doesn't seem as the highest prior right now: https://linear.app/comm/issue/ENG-724/think-about-passing-vectors-as-arguments Anyway, thanks for pointing this out!
48 ↗	(On Diff #9366)	https://linear.app/comm/issue/ENG-725/check-for-the-data-size-and-persistinblob-in-logitems-validate
55–57 ↗	(On Diff #9366)	As a rule of thumb, I decided to always go with strings when dealing with dynamo DB.
services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.h
36 ↗	(On Diff #9366)	right

add include vector - I added this in the header file

Harbormaster completed remote builds in B6716: Diff 9529.Feb 10 2022, 7:14 AM

@jimpo you're still blocking this, please, if possible, could you take a look?

Discussed offline with @jimpo, removing him from the reviewer list. Jim if you happen to find some time before this gets landed, your review will be always welcome!

This revision is now accepted and ready to land.Feb 14 2022, 3:31 AM

Thanks for following up with @jimpo offline, @karol-bisztyga!