Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow deletion of ephemeral objects before commit #7064

Merged
merged 6 commits into from
Oct 20, 2023

Conversation

jedelbo
Copy link
Contributor

@jedelbo jedelbo commented Oct 18, 2023

What, How & Why?

Likely to fix realm/realm-kotlin#1537

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • [ ] C-API, if public C++ API changed.

@@ -3385,6 +3385,17 @@ void Table::remove_object(ObjKey key)
{
Group* g = get_parent_group();

if (is_asymmetric()) {
REALM_ASSERT(g);
auto it = std::find_if(g->m_objects_to_delete.begin(), g->m_objects_to_delete.end(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: auto& objects_to_delete = g->m_objects_to_delete;

CHANGELOG.md Outdated Show resolved Hide resolved
@coveralls-official
Copy link

coveralls-official bot commented Oct 18, 2023

Pull Request Test Coverage Report for Build github_pull_request_280703

  • 46 of 47 (97.87%) changed or added relevant lines in 6 files are covered.
  • 101 unchanged lines in 14 files lost coverage.
  • Overall coverage decreased (-0.003%) to 91.596%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/realm/sync/subscriptions.cpp 9 10 90.0%
Files with Coverage Reduction New Missed Lines %
src/realm/cluster.cpp 2 80.25%
src/realm/db.cpp 2 93.64%
src/realm/table_view.cpp 2 94.18%
src/realm/uuid.cpp 2 97.06%
src/realm/sort_descriptor.cpp 3 93.7%
src/realm/sync/client.cpp 3 90.75%
src/realm/sync/network/network.cpp 3 89.87%
src/realm/sync/noinst/protocol_codec.hpp 3 76.72%
src/realm/sync/instructions.hpp 4 75.52%
src/realm/bplustree.cpp 7 76.74%
Totals Coverage Status
Change from base Build 1765: -0.003%
Covered Lines: 230549
Relevant Lines: 251703

💛 - Coveralls

@jedelbo jedelbo force-pushed the je/delete-asymmetric_object branch 2 times, most recently from 8a87283 to f167bd4 Compare October 18, 2023 16:10
@jedelbo jedelbo force-pushed the je/delete-asymmetric_object branch from f167bd4 to 8b3028e Compare October 19, 2023 06:09
src/realm/db.cpp Outdated
for (auto it : transaction.m_objects_to_delete) {
transaction.get_table(it.table_key)->remove_object(it.obj_key);
// Checking ttl would go here
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be lazy checking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I guess if we ever get this implemented, this would be the place to check which objects were due to be deleted. BTW if we skip recording the time the object was created, we can make this even more efficient. @danieltabacaru should we do that?

Copy link
Collaborator

@danieltabacaru danieltabacaru Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not saying it's a bad place to check, it's just that we would be relying on other things being committed. I am not sure if it's the same idea, but we don't really need the object keys, the table keys is enough (we can then clear the tables). we can optimize it further by also skipping storing the table keys by iterating through the tables and clearing the asymmetric ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course - the table keys are sufficient. Even better!

sub_sets->get_object_with_primary_key(Mixed{version_id}));
auto obj = sub_sets->get_object_with_primary_key(Mixed{version_id});
if (!obj) {
throw KeyNotFound("Subscription set not found");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should include the version_id in the message

std::lock_guard<std::mutex> lk(m_pending_notifications_mutex);
if (version_id < m_min_outstanding_version) {
return SubscriptionSet(weak_from_this(), version_id, SubscriptionSet::SupersededTag{});
}
throw;
throw KeyNotFound("Subscription set not found");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@jedelbo jedelbo merged commit 78b75be into master Oct 20, 2023
2 checks passed
@jedelbo jedelbo deleted the je/delete-asymmetric_object branch October 20, 2023 07:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RLM_ERR_NO_SUCH_OBJECT]: Key 'X' not found in <classname> when nullifying incoming links
2 participants