Fix Audit Message Logging Interceptor Race Condition #938
Conversation
mrzzy
requested review from
davidheryanto,
khorshuheng,
pyalex,
woop and
zhilingc
as code owners
|
/test test-end-to-end-redis-cluster |
|
/test test-end-to-end-batch |
|
/test test-end-to-end-redis-cluster |
|
/test test-end-to-end-batch-dataflow |
|
/test test-end-to-end |
mrzzy
changed the title
|
/test test-end-to-end-redis-cluster |
| call = | ||
| new SimpleForwardingServerCall<ReqT, RespT>(call) { | ||
| @Override | ||
| public void sendMessage(RespT message) { | ||
| // 2. Track the response & Log entry to audit logger | ||
| // Track the response & Log entry to audit logger | ||
| super.sendMessage(message); |
There was a problem hiding this comment.
Should this super.sendMessage(message); not be the last call that is made?
There was a problem hiding this comment.
This was done to ensure that logging code, as much as possible stays out of the way of the rest of message (ie exceptions in logging code do not disrupt message handling.) Going to try as last call to see if it fixes the concurrency issue.
There was a problem hiding this comment.
From my testing, moving the location of statements does not resolve the issue.
mrzzy
changed the title
|
/test test-end-to-end-batch-dataflow |
mrzzy
changed the title
feast-ci-bot
added
size/XXL
size/L
and removed
do-not-merge/work-in-progress
size/L
size/XXL
labels
…e if message cannot be recorded.
…to be called from multiple async calls
…not found by log4j2
…duced under load.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrzzy, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
/test test-end-to-end-redis-cluster |
Why we need it:
Under load the
GrpcMessageInterceptorwill throwIllegalStateExceptiondue to a race condition:GrpcMessageInterceptorexpects thatonMessage(),onHalfClose(),close()are called in that order.close()is being called beforeonMessage(), causing theMessageAuditLogEntryto be missingrequest, causingIllegalStateExceptionwhen.build()is called.What this PR does:
Fixes
GrpcMessageInterceptor's race condition by allowingclosed()to be called beforeonMessage()onMessage().Adds an integration test
CoreLoggingITto verify that Message Audit Logs are produced correctly.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: