New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add total_indexing_buffer/_in_bytes to nodes info API #18914
Add total_indexing_buffer/_in_bytes to nodes info API #18914
Conversation
@mikemccand what happens when the setting is missing? is there a default value? Can not seem to figure out where the setting will be enforced. Thanks. |
@litong01 the setting defaults to 10% of the JVM's heap. |
LGTM |
@@ -240,6 +252,11 @@ public void writeTo(StreamOutput out) throws IOException { | |||
super.writeTo(out); | |||
out.writeVInt(version.id); | |||
Build.writeBuild(build, out); | |||
if (totalIndexingBuffer == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we follow a different pattern here? should we write a boolean first and restore the null
value like other fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm agreed it would be nicer that serialization is symmetric
Thx @mikemccand for picking this up - I left some minor comments |
… indexing buffer value on the wire
@@ -78,12 +79,16 @@ | |||
@Nullable | |||
private IngestInfo ingest; | |||
|
|||
@Nullable | |||
private ByteSizeValue totalIndexingBuffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be null now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore that. It can :)
…ffer in NodeInfoStreamingTests; add @nullable annotation
indexingBuffer = null; | ||
} else { | ||
// pick a random long that sometimes exceeds an int: | ||
indexingBuffer = new ByteSizeValue(random().nextLong() & ((1L<<40)-1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
@bleskes I pushed another commit with your feedback, thanks! |
LGTM. Left one minor comment - no need for another review imo. Thanks again @mikemccand |
@bleskes suggested this in #18651.
When you set the indexing buffer (default: 10% of JVM's heap) it's somewhat non-trivial, since it has a buffer size and also min/max, so it would be nice to include what actual indexing buffer the node got
in nodes info.
I just added the indexing buffer size to
NodeInfo
, and expose it in the nodes info response astotal_indexing_buffer
(human readable) andtotal_indexing_buffer_in_bytes
.I also tried to improve the docs for the "core settings" returned by nodes info API.