Skip to content

Conversation

@PeteGillinElastic
Copy link
Member

No description provided.

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, I do think that is clearer, thanks for doing this. I also left a super-minor optional comment.

Comment on lines 314 to 315
"For data stream %s: %s based on [inc/dec cooldowns %s/%s, %d-%d threads, "
+ "write index %s has all-time/recent/peak loads %g/%g/%g, current shards %d, "
Copy link
Member

@dakrone dakrone Apr 4, 2025

Choose a reason for hiding this comment

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

This is super minor, so I leave it up to you whether you think it clarifies or not. Feel free to disregard my pedantry :)

When I think of / I think of it as alternative meanings for something (e.g., "my mother/care-taker" being verbalized as "my mother-slash-care-taker"). What do you think of something like:

Suggested change
"For data stream %s: %s based on [inc/dec cooldowns %s/%s, %d-%d threads, "
+ "write index %s has all-time/recent/peak loads %g/%g/%g, current shards %d, "
"For data stream %s: %s based on [inc/dec cooldowns %s/%s, %d-%d threads, "
+ "write index %s has [all-time|recent|peak] loads [%g|%g|%g], current shards %d, "

To clarify that they are corresponding values, instead of potentially implying that the loads are conflated and perhaps mixed?

(I realize the irony of replacing the / with | since | already connotes an "or" meaning, hence why this is so minor and up to you whether to make any changes)

Copy link
Member Author

Choose a reason for hiding this comment

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

See, I'm not sure that I'd find your version more clear!

I don't think that / is only used for alternative meanings. It's also used in dates, for example. If I said that a broadband speed test had a result of 800/500 Mbps for download/upload, would that be confusing?

I think I'm going to respectfully suggest that we've spent enough time on this, we appear to have reached a point where there's a subjective difference on the clearest punctuation without a clear answer, and I'm just going to mash the merge button...

Copy link
Member

Choose a reason for hiding this comment

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

That's totally reasonable! No worries at all, that's part of why I tried to couch it in optional language 😄

@PeteGillinElastic PeteGillinElastic marked this pull request as ready for review April 7, 2025 12:10
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Apr 7, 2025
@PeteGillinElastic PeteGillinElastic added >non-issue :Data Management/Data streams Data streams and their lifecycles and removed needs:triage Requires assignment of a team area label labels Apr 7, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Apr 7, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@PeteGillinElastic PeteGillinElastic merged commit 549fddb into elastic:main Apr 7, 2025
16 of 17 checks passed
@PeteGillinElastic PeteGillinElastic deleted the ES-10037-wordsmith-autosharding-logs branch April 7, 2025 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/Data streams Data streams and their lifecycles >non-issue Team:Data Management Meta label for data/management team v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants