Remove unused bwc variable from ApiKeyService #81964
Conversation
The variable lives in Subject since elastic#80926. The PR also replace string concatenation with text block to be consistent with other refactoring efforts (elastic#80751).
ywangd
added
>non-issue
:Security/Security
|
Pinging @elastic/es-security (Team:Security) |
| "enabled": true | ||
| } | ||
| } | ||
| }"""); |
There was a problem hiding this comment.
Do we want this formatting?
We push those bytes around the cluster and store them in ThreadContext, what's the impact of a 25% increase in the size of the string?
There was a problem hiding this comment.
Good question. I just thought it looks nicer in this format and didn't think of other implications. We don't store it in threadContext or sent it across nodes. But we do parse RoleDescriptor from it and more importantly computing message digest for it. So I think it's better not to have the format.
One alternative to reverting the change is to add a call of .replaceAll("[\\n ]", "") at the end of the text block. So it can leverage the better looking format without paying most of the runtime cost. This approach is not very future proof. But since the value is meant to be immutable for this variable. It is not really a concern.
I am happy to just simply revert given the main purpose of this PR is to getting rid of unused variable. What do you think?
There was a problem hiding this comment.
I decided to revert the format change.
3 participants
The variable lives in Subject since #80926. The PR also replace string
concatenation with text block to be consistent with other refactoring
efforts (#80751).