Skip to content

fix: add node health check to incr/decr before mutate operation #186

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.

Sign up for GitHub

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

Open
AustinWheel wants to merge 2 commits into
base: master
Choose a base branch
from
+15 −1
Open

fix: add node health check to incr/decr before mutate operation #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc942e1
fix: add node health check to incr/decr before mutate operation
AustinNflx Mar 3, 2026
75adcd2
fix: decr reconciliation picks min value; fast-fail inactive nodes in…
AustinNflx Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion evcache-core/src/main/java/com/netflix/evcache/EVCacheImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2961,7 +2961,7 @@ public long decr(String key, long by, long defaultVal, int timeToLive) throws EV
int index = 0;
for (EVCacheClient client : clients) {
vals[index] = client.decr(evcKey.getDerivedKey(client.isDuetClient(), client.getHashingAlgorithm(), client.shouldEncodeHashKey(), client.getMaxDigestBytes(), client.getMaxHashLength(), client.getBaseEncoder()), by, defaultVal, timeToLive);
if (vals[index] != -1 && currentValue < vals[index]) {
if (vals[index] != -1 && (currentValue == -1 || vals[index] < currentValue)) {
currentValue = vals[index];
if (log.isDebugEnabled()) log.debug("DECR : APP " + _appName + " current value = " + currentValue + " for key : " + key + " from client : " + client);
}
Expand Down
14 changes: 14 additions & 0 deletions evcache-core/src/main/java/com/netflix/evcache/pool/EVCacheClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,12 @@ public void reportWrongKeyReturned() {
private boolean ensureWriteQueueSize(MemcachedNode node, String key, EVCache.Call call) throws EVCacheException {
if (node instanceof EVCacheNode) {
final EVCacheNode evcNode = (EVCacheNode) node;
if (!evcNode.isAvailable(call)) {
incrementFailure(EVCacheMetricsFactory.INACTIVE_NODE, call);
if (log.isDebugEnabled()) log.debug("Inactive Node " + evcNode + " on " + call + " operation for app : " + appName
+ "; zone : " + zone + "; key : " + key);
return false;
}
int i = 0;
while (true) {
final int size = evcNode.getWriteQueueSize();
Expand Down Expand Up @@ -847,10 +853,18 @@ public Map<String, CachedData> getAllChunks(String key) throws EVCacheReadQueueE
}

public long incr(String key, long by, long defaultVal, int timeToLive) throws EVCacheException {
final MemcachedNode node = evcacheMemcachedClient.getEVCacheNode(key);
if (!ensureWriteQueueSize(node, key, Call.INCR)) {
Copy link
Contributor

@shy-1234 shy-1234 Mar 5, 2026

Choose a reason for hiding this comment

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

+1 to adding the check here. On the other hand, I am thinking we probably should do the evcNode.isAvailable(call) check first like in validateNodeForRead to fast fail instead of going through the retry wait loop if the node is already inactive. I couldn't think of a reason why we did this only for read APIs, but the same should also apply to write APIs IIUC. Any thought?

Copy link
Collaborator Author

@AustinWheel AustinWheel Mar 5, 2026

Choose a reason for hiding this comment

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

I think the goal is to not lose writes, the write queue is considerably larger than reads, and if a node went out of discovery for some small time and it's write queue isn't full, we could still add operations which it could process after returning

Copy link
Contributor

@shy-1234 shy-1234 Mar 5, 2026

Choose a reason for hiding this comment

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

That's a good point. However, I wonder is that better than just fast fail those writes. EVCache writes go to multiple zones/replicas. A failed write to one node doesn't mean data loss. The failed result will be reflected in the latch count and the client's LatchPolicy will check if they can tolerant one or some failure.

And for a high throughput workload, there can be lots of request threads retrying/waiting (threads are blocked on sleep) before the connection can be rebuilt (there is also a backoff time between each reconnect, which makes this wait even longer I think).

Copy link
Collaborator Author

@AustinWheel AustinWheel Mar 6, 2026

Choose a reason for hiding this comment

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

okay yeah I think it could be useful for incr/decr operations, but should such a change be added to all write ops?

return -1;
}
return evcacheMemcachedClient.incr(key, by, defaultVal, timeToLive);
}

public long decr(String key, long by, long defaultVal, int timeToLive) throws EVCacheException {
final MemcachedNode node = evcacheMemcachedClient.getEVCacheNode(key);
if (!ensureWriteQueueSize(node, key, Call.DECR)) {
return -1;
}
return evcacheMemcachedClient.decr(key, by, defaultVal, timeToLive);
}

Expand Down