Introduce BoundedDelimitedStringCollector
#124303
Conversation
An issue with `Strings#collectionToDelimitedStringWithLimit` is that you need to collect all the items together up front first, even if you're going to throw most of them away. This commit introduces `BoundedDelimitedStringCollector` which allows to accumulate the items one-at-a-time instead.
DaveCTurner
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
|
||
| if (state != batchExecutionContext.initialState()) { | ||
| var reason = new StringBuilder(); | ||
| Strings.collectionToDelimitedStringWithLimit(results, ",", "lazy bulk rollover [", "]", 1024, reason); |
There was a problem hiding this comment.
I just realized the prefix and suffix params don't work as I thought they would. They're added to every item, whereas I thought they would only be added once at the start and once at the end respectively. I see that the two usages in rollover and lazy rollover are the only usages that specify a (non-empty) prefix and suffix, and it looks to me like it doesn't really make sense to wrap every result with the string with lazy bulk rollover [...]; it would make more sense to wrap all the results with the string lazy bulk rollover [result1, result2].
Assuming that we'd fix those two use cases, there are no use cases of Strings.collectionToDelimitedStringWithLimit that pass non-empty strings for the prefix and suffix, so I think we can just remove those two parameters. What do you think?
There was a problem hiding this comment.
ha yeah that caught me out when re-implementing it here too
There was a problem hiding this comment.
let me open a PR to do that first, it'll be noisy here
nielsbauman
left a comment
nielsbauman
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the iterations, David :)
DaveCTurner
added
auto-merge-without-approval
💔 Backport failed
You can use sqren/backport to manually backport by running |
An issue with `Strings#collectionToDelimitedStringWithLimit` is that you need to collect all the items together up front first, even if you're going to throw most of them away. This commit introduces `BoundedDelimitedStringCollector` which allows to accumulate the items one-at-a-time instead. Backport of elastic#124303 to `8.x`
|
backport is #124382 |
An issue with `Strings#collectionToDelimitedStringWithLimit` is that you need to collect all the items together up front first, even if you're going to throw most of them away. This commit introduces `BoundedDelimitedStringCollector` which allows to accumulate the items one-at-a-time instead. Backport of #124303 to `8.x`
An issue with `Strings#collectionToDelimitedStringWithLimit` is that you need to collect all the items together up front first, even if you're going to throw most of them away. This commit introduces `BoundedDelimitedStringCollector` which allows to accumulate the items one-at-a-time instead.
3 participants
An issue with
Strings#collectionToDelimitedStringWithLimitis that youneed to collect all the items together up front first, even if you're
going to throw most of them away. This commit introduces
BoundedDelimitedStringCollectorwhich allows to accumulate the itemsone-at-a-time instead.