search

tumblr / colossus

I/O and Microservice library for Scala
Apache License 2.0
1.14k stars 96 forks source link

adding logging on failed insert #512

Closed amotamed closed 7 years ago

amotamed commented 7 years ago

@DanSimon @benblack86 @aliyakamercan

Logging the error when serviceclient's queue is full

benblack86 commented 7 years ago

Instead of logging, I think it would be better to return failure. Failure the request?

DanSimon commented 7 years ago

Hmm, is there a specific reason or symptom why this is being added? To me it looks like isAdded can never be false. The canPush method will return false if msgq is full, and being full is the only reason why msgq.enqueue would return false.

Also, ServiceClient does properly handle the return value of push here, which ends up failing the request with a ClientOverloadedException.

amotamed commented 7 years ago

@benblack86 I'm not sure if we want to do a full error or warn. I'm totally open to discussing it

@DanSimon If we have a full queue of items, enqueue doesn't add the item and returns false. This is to fix an issue we saw in Refrog. Adding a Memcached node (invalidating a fifth of the cache) caused a ton of backfill requests that resulted in the backfill not finishing.

In other news: I'm officially (still with a ton of boxes) moved

DanSimon commented 7 years ago

Right, but if the queue is full than the if-statement this code is inside will also be false, so we know for sure that it will only enqueue when msgq is not full. If you look at the canPush method it checks to see if the queue is full and returns false if it is.

amotamed commented 7 years ago

I totally see your point Dan, I'm closing this since I haven't been able to put the time to reproduce what we were seeing on the redis node rotation