Add retries to JavaEntityClient:deleteReferencesTo #8268
Conversation
joshuaeilers
force-pushed
the
je--retry-deleteEntityReferences
branch
from
daa1a2a
to
fa60ed6
Compare
joshuaeilers
force-pushed
the
je--retry-deleteEntityReferences
branch
from
fa60ed6
to
d1aefcb
Compare
There was a problem hiding this comment.
nice! this is looking great. here's an example PR doing a very similar thing that you can use as a model to add a test or two: https://github.com/datahub-project/datahub/pull/8172/files
| try { | ||
| return block.get(); | ||
| } catch (Throwable ex) { | ||
| MetricUtils.counter(JavaEntityClient.class, "exception" + MetricUtils.DELIMITER + ex.getClass().getName().toLowerCase()).inc(); |
There was a problem hiding this comment.
Thoughts on this counter?
There was a problem hiding this comment.
ooo love the counter. it might be nice to know what exactly is failing instead of just the name of the exception class.
what do you think about passing a method name or something to track with this counter into withRetry? so here it could be like delete_references_to_failures or something - and it could be optional so if they don't provide a metric name we could just pass in the exception class like you're doing
| try { | ||
| return block.get(); | ||
| } catch (Throwable ex) { | ||
| MetricUtils.counter(JavaEntityClient.class, "exception" + MetricUtils.DELIMITER + ex.getClass().getName().toLowerCase()).inc(); |
There was a problem hiding this comment.
ooo love the counter. it might be nice to know what exactly is failing instead of just the name of the exception class.
what do you think about passing a method name or something to track with this counter into withRetry? so here it could be like delete_references_to_failures or something - and it could be optional so if they don't provide a metric name we could just pass in the exception class like you're doing
Since we suspect failing calls to deleteReferencesTo (there may be other places) this PR adds retry logic. Normally, retries would not be the best thing to do here, but since we don't have a great way to rollback resolvers on failure at the moment this may help reduce the frequency of data inconsistencies. Also address other places we may not be catching all exceptions in runAsync so we get better logging.
Could use some pointers on testing / appropriate way to structure this.
Checklist