Skip to content
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.

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

Add method to extract all errors of a partial failure status at once #626

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

markusheiden
Copy link

This is useful because getting all errors via their operation index is inefficient. In #getGoogleAdsErrors(long, GoogleAdsFailureT) the error paths for all failures are build just to pick those for the requested operation index. That leads creation of many ErrorPath objects for all not requested operation indexes which. Those are immediately discarded what leads to avoidable GC pressure.

This can already be achieved with the current API, but I think it belongs into the error util. The code (Java 11) I currently use as supplement:
status.getDetailsList().stream()
.map(errorUtils::getGoogleAdsFailure)
.map(errorUtils::getGoogleAdsErrors)
.flatMap(List::stream)
.collect(toSet());

This is useful because getting all errors via their operation index is inefficient. In #getGoogleAdsErrors(long, GoogleAdsFailureT) the error paths for all failures are build just to pick those for the requested operation index. That leads creation of many ErrorPath objects for all not requested operation indexes which. Those are immediately discarded what leads to avoidable GC pressure.

This can already be achieved with the current API, but I think it belongs into the error util. The code (Java 11) I currently use as supplement:
status.getDetailsList().stream()
  .map(errorUtils::getGoogleAdsFailure)
  .map(errorUtils::getGoogleAdsErrors)
  .flatMap(List::stream)
  .collect(toSet());
@markusheiden
Copy link
Author

markusheiden commented May 19, 2022

When implementing this PR I wondered why the errors in AbstractErrorUtils#getGoogleAdsErrors(long, GoogleAdsFailureT) are deduplicated via if (!result.contains(error)) result.add(error)? Using a java.util.Set in this case would IMO be more efficient because the duplicate check performance currently decreases with the length of the java.util.List used for result. This IMO needs an incompatible API change of AbstractErrorUtils though.

This applies to other methods too, e.g. AbstractErrorUtils#getFailedOperationIndices where .distinct().collect(toList()) is used what first internally creates a set to convert it to a list afterwards.

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