Skip to content

Conversation

@alexeyqu-fb
Copy link
Contributor

This becomes important when the original exception (e.g. rate limit exceeded) is shadowed with a wrapping exception somewhere, so the toString doesn't contain the important information anymore.

Using ExceptionUtils.getStackTrace from lang3.exception is one of the ways to look into the stack trace -- however, I am open to suggestions.

cc @xokker

if (input.matches(regex)) {
return true;
}
}
Copy link
Collaborator

@jzacsh jzacsh Oct 4, 2022

Choose a reason for hiding this comment

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

does leaving the previous loop seem like a micro-optimization? should we just delete it for now and add something like this (or something else) in later, if we do performance testing and find this area problematic?

EDIT: I see the loop before is actually probably very different (first loop vs. second loop). so maybe just add a comment block atop both loops explaining (eg: // try to match on throwable's faster-to-compute string and // see if maybe the fuller stacktrace reveals any matches)?

Copy link
Collaborator

@jzacsh jzacsh left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with this class, but maybe it'd increase confidence if we started adding unit test coverage? Maybe not to this file, but to its one and only user (as far as I can tell): RetryStartegyLibrary itself? eg: maybe then it'd be easy (for future us) to see this PR was just adding a new test case?

@CLAassistant
Copy link

CLAassistant commented Jun 26, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ xokker
❌ alexeyqu-fb
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

4 participants