Skip to content

Conversation

@DaveCTurner
Copy link
Contributor

In principle this utility class lets us format potentially-large numbers
nicely in API responses, but in practice it is wholly unused for that
purpose. Nor is it used to accept user input (e.g. setting values) that
could potentially be large. In the few places it appears in the codebase
we can just use Long instead.

In principle this utility class lets us format potentially-large numbers
nicely in API responses, but in practice it is wholly unused for that
purpose. Nor is it used to accept user input (e.g. setting values) that
could potentially be large. In the few places it appears in the codebase
we can just use `Long` instead.
@DaveCTurner DaveCTurner requested a review from a team as a code owner September 17, 2025 07:34
@DaveCTurner DaveCTurner added >non-issue :Core/Infra/Core Core issues without another label v9.2.0 labels Sep 17, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Sep 17, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)


if (poolInfo != null) {
if (poolInfo.getQueueSize() != null) {
maxQueueSize = poolInfo.getQueueSize().singles();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the only place we could possibly have used SizeValue for nice formatting of large numbers in a cat API, but the .singles() call forces it to be a pure integer instead.

private final int max;
private final TimeValue keepAlive;
private final SizeValue queueSize;
private final Long queueSize;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only possible observable change is that the Info#toString() now always shows the bare integer rather than something like 10k for larger numbers. I'm not sure this is actually visible to end-users anywhere and honestly IMO that's an improvement.

Copy link
Contributor

@mosche mosche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, lgtm 💯

}
}
if (value instanceof SizeValue v) {
String resolution = request.param("size");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that would even conflict with usage of size in cat.transforms and cat.ml_trained_models 💥

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does.

@DaveCTurner DaveCTurner merged commit 1accaf3 into elastic:main Sep 17, 2025
34 checks passed
@DaveCTurner DaveCTurner deleted the 2025/09/17/remove-SizeValue branch September 17, 2025 09:53
mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Sep 17, 2025
In principle this utility class lets us format potentially-large numbers
nicely in API responses, but in practice it is wholly unused for that
purpose. Nor is it used to accept user input (e.g. setting values) that
could potentially be large. In the few places it appears in the codebase
we can just use `Long` instead.
szybia added a commit to szybia/elasticsearch that referenced this pull request Sep 17, 2025
* upstream/main:
  Add additional logging to make spotting stats issues easier (elastic#133972)
  [ESQL] Clean up ESQL enrich landing page (elastic#134820)
  ES|QL: Make kibana docs for Query settings more consistent (elastic#134881)
  Add file extension metadata to cache miss counter from SharedBlobCacheService (elastic#134374)
  Add IT for num_reduced_phases with batched query execution (elastic#134312)
  Remove `SizeValue` (elastic#134871)
gmjehovich pushed a commit to gmjehovich/elasticsearch that referenced this pull request Sep 18, 2025
In principle this utility class lets us format potentially-large numbers
nicely in API responses, but in practice it is wholly unused for that
purpose. Nor is it used to accept user input (e.g. setting values) that
could potentially be large. In the few places it appears in the codebase
we can just use `Long` instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants