[FS-1588] No proposals after deleting a subconversation#3123
[FS-1588] No proposals after deleting a subconversation#3123mdimjasevic merged 6 commits intomlsfrom
Conversation
zebot
added
the
ok-to-test
mdimjasevic
force-pushed
the
fs-1588/no-proposals-after-deleting-subconv
branch
from
31d4d50 to
20be258
Compare
mdimjasevic
force-pushed
the
fs-1588/no-proposals-after-deleting-subconv
branch
2 times, most recently
from
e4268a8 to
201ea64
Compare
mdimjasevic
force-pushed
the
fs-1588/no-proposals-after-deleting-subconv
branch
from
d904e39 to
2f2d0fa
Compare
| msub <- embedClient (selectSubConversation convId subConvId) | ||
| for_ msub $ deleteAllProposals . cnvmlsGroupId . scMLSData |
There was a problem hiding this comment.
Why explicitly deleting the pending proposals? Essentially, this is buying Cassandra space and paying with users' time. I'd argue that users' time is more valuable. Also, the Cassandra records have a TTL and will be deleted eventually and they will not appear in a recreated subconversation, since the new subconversation's groupID is a different one than the one of the subconversation before. This is also demonstrated by your test which passes even without changes in business logic.
There was a problem hiding this comment.
Sorry for not seeing and commenting this when I checked this PR previously!
There was a problem hiding this comment.
Your reasoning is perfectly fine, but if we're to accept this argument, I'd prefer to hear arguments for the deleting a subconversation operation to be on a critical path, ideally backed by numbers showing this DB clean-up noticeably slows down the response time. We could let the proposals linger until they're garbage-collected, but to me this seems not so great.
I expect this delete DB statement to be deleting one or two records on average, and in rare case a handful of them. When this is put next to so many DB operations we do anyway in this endpoint handler, I'd argue the introduced time overhead is negligible.
There was a problem hiding this comment.
Can you raise this in the chat? I would argue that removing the records was not forgotten, but deliberately skipped. As to the critical path, ending a call should be done as quick as possible. What would you do if you hang up and nothing happens for a moment? We need a second opinion/review if you insist on cleaning up the non-user-facing database in exchange for user-facing latency.
There was a problem hiding this comment.
Since proposals have a TTL, there isn't much point to delete them, unless you really want them gone for semantic reasons. Note that deleting doesn't actually reclaim any space in cassandra, AFAIU.
There was a problem hiding this comment.
Ok, I'll revert the change in the application code. The test can stay as it doesn't hurt to assert there are no leftovers.
|
Any further comments? |
4 participants
This makes sure the database is cleaned up from leftover backend-generated remove proposals once a subconversation is deleted. Due to our test setup, this is not so straightforward to test, and doing a database test is not something we have been doing lately.
The test added by the PR just makes sure a recreated subconversation has no leftover proposals, but that holds because the recreated subconversation has a fresh group ID, hence fetching proposals for that new group ID yields no results anyway.
Tracked by https://wearezeta.atlassian.net/browse/FS-1588.
Checklist
changelog.d