[services] consistently return when trying to remove non-existent item
ClosedPublic
Actions

Authored by • karol on May 11 2022, 2:19 AM.

Details

Reviewers

• max
tomek

Commits

rCOMM687ca39ad777: [services] consistently return when trying to remove non-existent item

Summary

Depends on D3990

Raised in https://phabricator.ashoat.com/D3990#112037

We should be consistent and throw if someone tries to remove an item that does not exist. That clearly means the logic's invalid.

Test Plan

cd services
yarn run-tunnelbroker-service
yarn test-blob-service-dev-mode
yarn test-backup-service-dev-mode

Diff Detail

Repository

rCOMM Comm

Branch

services-lib-tb

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.May 11 2022, 2:19 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptMay 11 2022, 2:19 AM

• karol edited the summary of this revision. (Show Details)May 11 2022, 2:20 AM

• karol edited the test plan for this revision. (Show Details)

• karol added reviewers: • max, tomek.

• karol added a parent revision: D3990: [services] Lib - Tunnelbroker - Use database manager.

• karol mentioned this in D3990: [services] Lib - Tunnelbroker - Use database manager.

Harbormaster completed remote builds in B8995: Diff 12532.May 11 2022, 2:24 AM

• karol requested review of this revision.May 11 2022, 2:24 AM

I think ideally the changes in services/tunnelbroker/src/Database/DatabaseManager.cpp would've been applied in D3990. See feedback here

• karol added a child revision: D4013: [services] Rename PrimaryKey to PrimaryKeyDescriptor.May 12 2022, 2:05 AM

rebase

Harbormaster completed remote builds in B9032: Diff 12573.May 12 2022, 2:17 AM

That clearly means the logic's invalid.

No. It might also mean that we tried to delete the same item twice, which is fine. Take a C++ map as an example: when we erase an item twice, it doesn't throw an error.

We can consider returning some info about the presence of an item, but I still wouldn't call this an error.

This revision now requires changes to proceed.May 12 2022, 2:42 AM

Hmm, maybe you're right. I'm going to wait until we have terms in D3990. Then we can either land or abandon this one.

return instead of throwing

Harbormaster failed remote builds in B9075: Diff 12621!May 13 2022, 2:24 AM

• karol retitled this revision from [services] Throw when trying to remove non-existent item to [services] consistently return when trying to remove non-existent item.May 13 2022, 2:24 AM

tomek accepted this revision.May 13 2022, 2:29 AM

I'm not a big fan of silently returning if expected items do not exist but accepting this as we agreed before in D3990.

This revision is now accepted and ready to land.May 13 2022, 4:26 AM

This revision was landed with ongoing or failed builds.May 13 2022, 4:30 AM

Closed by commit rCOMM687ca39ad777: [services] consistently return when trying to remove non-existent item (authored by • karol). · Explain Why

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMM687ca39ad777: [services] consistently return when trying to remove non-existent item.