Do not create String instances in 'Strings' methods accepting StringBuilder #22907
Conversation
|
Can one of the admins verify this patch? |
| @@ -90,12 +90,12 @@ public void shutdown() throws Exception { | |||
| public void testCorsConfig() { | |||
| final Set<String> methods = new HashSet<>(Arrays.asList("get", "options", "post")); | |||
| final Set<String> headers = new HashSet<>(Arrays.asList("Content-Type", "Content-Length")); | |||
| final String suffix = randomBoolean() ? " " : ""; // sometimes have a leading whitespace between comma delimited elements | |||
| final String prefix = randomBoolean() ? " " : ""; // sometimes have a leading whitespace between comma delimited elements | |||
There was a problem hiding this comment.
Can you please undo this unrelated change?
There was a problem hiding this comment.
This value is passed as 'prefix' parameter to the collectionToDelimitedString(coll, delim, prefix, suffix).
And since that method was affected by this change I thought it might be a good idea to also eliminate this possible source of confusion.
But sure, I'll undo it tomorrow.
There was a problem hiding this comment.
I think it's a good change and not completely unrelated. Why undo it @rjernst ?
There was a problem hiding this comment.
I'm fine with it, given the explanation. I only saw that it seemed to be a variable name change that was unnecessary.
|
@elasticmachine test this please |
|
@elasticmachine retest this please, just once, please please please |
|
The build was failing due to some unresolvable dependency. I didn't touch any Gradle files though. |
|
@sabi0 We've had issues with our PR CI system. I'll try it once more, otherwise I will run tests manually. @elasticmachine test this please |
|
If your CI is back to normal would you mind pulling this in? |
|
@sabi0 I'm sorry I lost track of this! I just tested and it is good. I will merge now. |
|
Thanks @sabi0! |
* master: (54 commits) Keep the pipeline handler queue small initially Do not create String instances in 'Strings' methods accepting StringBuilder (elastic#22907) Tests: fix AwsS3ServiceImplTests Remove abstract InternalMetricsAggregation class (elastic#23326) Add BulkRequest support to High Level Rest client (elastic#23312) Wrap getCredentials() in a doPrivileged() block (elastic#23297) Respect promises on pipelined responses Align REST specs for HEAD requests Remove unnecessary result sorting in SearchPhaseController (elastic#23321) Fix SamplerAggregatorTests to have stable and predictable docIds Tests: Ensure multi node integ tests wait on first node Relocate a comment in HttpPipeliningHandler Add comments to HttpPipeliningHandler [TEST] Fix incorrect test cluster name in cluster health doc tests Build: Change location in zip of license and notice inclusion for plugins (elastic#23316) Script: Fix value of `ctx._now` to be current epoch time in milliseconds (elastic#23175) Build: Rework integ test setup and shutdown to ensure stop runs when desired (elastic#23304) Handle long overflow when adding paths' totals Don't set local node on cluster state used for node join validation (elastic#23311) Ensure that releasing listener is called ...
Imagine you are doing something like this:
And before you know it, you've just allocated two extra 5 MB
Stringinstances on the heap.