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.

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

OpenSearch Sink: Add log messages when there is no exception #3532

Merged
merged 2 commits into from
Oct 31, 2023
Merged

OpenSearch Sink: Add log messages when there is no exception #3532

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eba6f67
Add log messages when there is exception
kkondaka Oct 19, 2023
a8ec25c
Addressed review comments
kkondaka Oct 30, 2023
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
13 changes: 13 additions & 0 deletions ...h/src/main/java/org/opensearch/dataprepper/plugins/sink/opensearch/BulkRetryStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,13 @@ private BulkOperationRequestResponse handleRetriesAndFailures(final Accumulating
if (doRetry) {
if (retryCount % 5 == 0) {
LOG.warn("Bulk Operation Failed. Number of retries {}. Retrying... ", retryCount, e);
if (Objects.isNull(e)) {
Copy link
Member

Choose a reason for hiding this comment

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

The reason isNull exists is to help with streams. I think outside of a stream, we should use x == null.

for (final BulkResponseItem bulkItemResponse : bulkResponse.items()) {
if (Objects.nonNull(bulkItemResponse.error())) {
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that this will be non-null for successful records? Did you test and see no log here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean to say "it will be NULL for successful records"?
Don't know how to test this path easily. But based on the following existing code, I think it will be set to null for successful records.

if (bulkItemResponse.error() == null) {
                    sentDocumentsOnFirstAttemptCounter.increment();
 }

LOG.warn("operation = {}, error = {}", bulkItemResponse.operationType(), bulkItemResponse.error());
}
}
}
}
bulkRequestNumberOfRetries.increment();
return new BulkOperationRequestResponse(bulkRequestForRetry, bulkResponse);
Expand All @@ -248,7 +255,13 @@ private BulkOperationRequestResponse handleRetriesAndFailures(final Accumulating
}

private void handleFailures(final AccumulatingBulkRequest<BulkOperationWrapper, BulkRequest> bulkRequest, final BulkResponse bulkResponse, final Throwable failure) {
LOG.warn("Bulk Operation Failed.", failure);
Copy link
Member

Choose a reason for hiding this comment

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

Do we want the full stack trace here? We know where the exception is thrown from right?

Or should this be the following?

 LOG.warn("Bulk Operation Failed. {}", failure.getMessage());

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function may be called from multiple paths, right?

if (Objects.isNull(failure)) {
for (final BulkResponseItem bulkItemResponse : bulkResponse.items()) {
if (Objects.nonNull(bulkItemResponse.error())) {
Copy link
Member

Choose a reason for hiding this comment

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

Again, let's use bulkItemResponse.error() == null.

LOG.warn("operation = {}, error = {}", bulkItemResponse.operationType(), bulkItemResponse.error());
}
}
handleFailures(bulkRequest, bulkResponse.items());
} else {
handleFailures(bulkRequest, failure);
Copy link
Member

Choose a reason for hiding this comment

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

The problem is that right now we only hit this block I think (or at least that is the case when max_retries is reached). But as long as the failure is actually useful this makes sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new change, prints "failure" in both cases. If it is non-null, it will be printed. If it is non-null, I think it would be useful.

Expand Down
Loading