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

feat: do NOT add delete entries in commit log when expired keys are deleted #1713

Merged
merged 37 commits into from
Feb 21, 2024

Conversation

srieteja
Copy link
Contributor

@srieteja srieteja commented Dec 20, 2023

closes #1660
- What I did

  • Do not add CommitEntry for Delete Op on expired keys (disabled for now, will be enabled later)
  • Introduced an optional param in scheduleKeyExpireTask() called runInterval which takes Duration and schedules the expiryTask at this timeInterval. (This is an improvement as previously this method would accept only time in mins and in type String)

- How I did it

  • Introduce an optional named param in deleteExpiredKeys() called skipCommit, this decides whether a commit should be skipped when an expired key is deleted. [defaults to false; meaning commit will be created]
  • Based on the method param above, set the skipCommit flag while calling remove() from deleteExpiredKeys() [in hiveKeyStore and notificationKeyStore]

- How to verify it

  • new unit tests added should verify the behaviour of expired keys w.r.t. the changes

- Description for the changelog

feat: do NOT add delete entries in commit log when expired keys are deleted

murali-shris
murali-shris previously approved these changes Dec 26, 2023
@gkc
Copy link
Contributor

gkc commented Jan 2, 2024

@srieteja We have a chicken and egg scenario here; until we can be sure that most apps are using the latest AtClient, those clients will still be dependent on the commit log and sync responses to trigger deletion on the client side.

Therefore for now we need to

  • add a flag to the atServer's config for this feature, defaulted to false (not enabled) for now
  • and add a way of setting that flag dynamically for testing purposes (add it to ModifiableConfigs, add a subscriber somewhere, etc etc)
  • and modify tests so they verify both the current behaviour with the flag not enabled, and the new behaviour with the flag enabled
  • and add a test to assert that the flag is by default not enabled

At some time in the future, we can change the config to enable this feature by default. I'd suggest create a ticket for that and reference that new ticket in the description of #1660 and we'll keep assigning the ticket to the "sprint after next" until we think we're ready to go.

Make sense? cc @murali-shris @sitaram-kalluri

@gkc gkc marked this pull request as draft January 2, 2024 18:19
Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

See comment I left

@srieteja
Copy link
Contributor Author

srieteja commented Jan 4, 2024

Added flags in hive_manages/scheduleKeyExpiryTask() and hive_keystore/deleteExpiredKeys() to enable the feature being written in this PR. Also introduced config in AtSecondaryServer/AtSecondaryConfig and passed the same to the hive_manager/keystore to enable the feature in the PR (see #1727)

@srieteja
Copy link
Contributor Author

I have tested the functionality and how this feature interacts with the client. @purnimavenkatasubbu has tested this along with #1727 and atsign-foundation/at_client_sdk#1187 with buzz and wavi. Everything seems to be working fine, Ready to be reviewed

@srieteja srieteja requested a review from VJag February 20, 2024 09:51
@gkc
Copy link
Contributor

gkc commented Feb 20, 2024

@srieteja in the PR description you say that you

  • set the skipCommit flag to true in deleteExpiredKeys() [in hiveKeyStore and notificationKeyStore]

that is only true for notificationKeyStore. In hiveKeyStore deleteExpiredKeys takes an "optimizeCommits" flag

@srieteja srieteja requested a review from gkc February 21, 2024 10:15
Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work on the tests

@gkc
Copy link
Contributor

gkc commented Feb 21, 2024

@srieteja in the PR description you say that you

  • set the skipCommit flag to true in deleteExpiredKeys() [in hiveKeyStore and notificationKeyStore]

that is only true for notificationKeyStore. In hiveKeyStore deleteExpiredKeys takes an "optimizeCommits" flag

@srieteja Please can you correct the PR description so it is accurate

@gkc gkc dismissed sitaram-kalluri’s stale review February 21, 2024 11:19

Requested change was addressed

@gkc gkc merged commit 7e1584a into trunk Feb 21, 2024
21 checks passed
@gkc gkc deleted the delete_commit_nosync branch February 21, 2024 11:19
@srieteja
Copy link
Contributor Author

Updated the PR description @gkc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve commit log handling for expired (expiresAt has passed) records
4 participants