Conversation
* if we have enough logs for a batch, keep sending batched logs * otherwise wait for the interval to elapse, then send what we have
mcculls
added
type: bug
| logsReady.poll(waitNanos, TimeUnit.NANOSECONDS); | ||
| } | ||
| } catch (InterruptedException ignore) { | ||
| break; |
There was a problem hiding this comment.
As general practice, we should reset the interrupt status after catching InterruptedException.
There was a problem hiding this comment.
In general yes - but only if we want to preserve and report the interrupted status to callers, which is not the case here.
In this situation we want to bail out of the method and return what we've batched so far (so it isn't dropped.) The caller then may decide to invoke waitForLogs again, and we don't want to have the interrupted status kept as then poll would immediately fail again.
We could potentially let the InterruptedException bubble up, but that would lead to a loss of logs if we're interrupting because of shutdown, where we want to interrupt any blocking while waiting for enough logs for the batch but still have one last attempt to flush what we have.
There was a problem hiding this comment.
I'll add a comment to make this particular situation clear for future devs
| thread.interrupt(); | ||
| try { | ||
| thread.join(1_000); | ||
| } catch (InterruptedException ignore) { |
There was a problem hiding this comment.
I'm okay with foregoing reseting interrupt here, since we're mid shutdown.
However, a comment explaining that would be good.
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
Looks good
Some minor comments about interrupt handling
2 participants
What Does This Do
Motivation
Better alignment with OpenTelemetry SDK behaviour: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessor.java#L206
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.