Refactor SlowLogFieldProvider#140397
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
mark-vieira
left a comment
There was a problem hiding this comment.
Some minor comments but otherwise LGTM.
| /** | ||
| * Do we want any user authentication context? | ||
| */ | ||
| private volatile boolean includeUserInformation; |
There was a problem hiding this comment.
I don't think it's really necessary to mark this volatile. Settings updates are by nature async, so this being somewhat eventually consistent is fine IMO and probably not worth the perf hit on reading this field on every log entry.
There was a problem hiding this comment.
Since the cluster state changes would be updating this variable, I am concerned the checks may get an old value... probably not likely to matter that much in real scenario, but might make some tests flaky. I can drop it and see if anything breaks.
There was a problem hiding this comment.
Ah, I hadn't considered test stability. You might be right. In production it doesn't really matter, it just means there may be some insignificant delay until the setting change actually takes hold.
test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java
Outdated
Show resolved
Hide resolved
* Refactor SlowLogFieldProvider
Refactoring includes: