Skip to content

Conversation

@osteensco
Copy link
Contributor

This PR addresses #151.

  • refactored NewSugarDB calls in tests to allow for easier config changes.
  • added test to Expire in generic module test to check that keys expired as expected.
  • added TTLHeap and HeapItem types.
  • added necessary methods for TTLHeap to implement the container/heap interface.
  • removed eviction sample from sugardb config as it's no longer required.
  • made some quality of life adjustments to test_env containers

@osteensco
Copy link
Contributor Author

Notes:

  • This PR still needs to be caught up with main.
  • The Test_EvictExpiredTTL in the sugardb Test_Cluster suite is timing out. There are various print statements left in since debugging this is a WIP.
  • The root cause of the bug with the above mentioned test has been narrowed down to the follower nodes are receiving the deletion requests from the leader and sending a response back to the leader, but the leader is not receiving this response for some reason.

@osteensco
Copy link
Contributor Author

FYI, probably gonna hold off and refactor this after #164 gets done.

@osteensco
Copy link
Contributor Author

I believe I found the root cause to the issue with this PR.
We can observe the deletion requests are sent to all nodes and are successful:

image

However, from what I understand, a deletion request is also sent to the leader node. I believe this request specifically is what's causing the response to get hung up. This causes a deadlock because in SugarDB.evictKeysWithExpiredTTL() we have to engage the storeLock as well as the keysWithExpiry lock. BUT these two locks are engaged in the HandlerFuncParams.DeleteKey() function.

This seems to be a catch 22, since these locks are needed in both places.

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.

1 participant