-
Notifications
You must be signed in to change notification settings - Fork 0
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
Stats API bug fix #7
Conversation
The stats API broadcasts requests to all nodes and renders node responses using toXContent. For the local node, the stats API's calls toXContent on the node response directly. For remote node, the coordinating node gets a serialized content from ADStatsNodeResponse.writeTo, deserializes the content and renders the result using toXContent. ADStatsNodeResponse.writeTo uses StreamOutput::writeGenericValue that only recognizes built-in types (e.g., String or primitive types). It throws exceptions for types such as java.time.Instant and Entity. This PR fixes the bug by converting non-built-in types to built-in types in node stats response. This PR also removes model id from Entity.toString as it is random across different runs and keeps only attributes. This can help locate the same node in hash ring. Testing done: * Verified stats API response on a multi-node cluster for a multiple-category field detector. * Added unit tests.
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.
LGTM - can you confirm the issue with ADStatsNodeResponse.writeTo
is limited to responses from the stats API? No other responses have risk of returning non-built-in types?
confirmed this is the only place using StreamOutput::writeGenericValue. |
This PR is a conglomerate of the following PRs. #60 #64 #65 #67 #68 #69 #70 #71 #74 #75 #76 #77 #78 #79 #82 #83 #84 #92 #94 #93 #95 kaituo#1 kaituo#2 kaituo#3 kaituo#4 kaituo#5 kaituo#6 kaituo#7 kaituo#8 kaituo#9 kaituo#10 This spreadsheet contains the mappings from files to PR number (bug fix in my AD fork and tests are not included): https://gist.github.com/kaituo/9e1592c4ac4f2f449356cb93d0591167
…ject#121) This PR is a conglomerate of the following PRs. opensearch-project#60 opensearch-project#64 opensearch-project#65 opensearch-project#67 opensearch-project#68 opensearch-project#69 opensearch-project#70 opensearch-project#71 opensearch-project#74 opensearch-project#75 opensearch-project#76 opensearch-project#77 opensearch-project#78 opensearch-project#79 opensearch-project#82 opensearch-project#83 opensearch-project#84 opensearch-project#92 opensearch-project#94 opensearch-project#93 opensearch-project#95 kaituo#1 kaituo#2 kaituo#3 kaituo#4 kaituo#5 kaituo#6 kaituo#7 kaituo#8 kaituo#9 kaituo#10 This spreadsheet contains the mappings from files to PR number (bug fix in my AD fork and tests are not included): https://gist.github.com/kaituo/9e1592c4ac4f2f449356cb93d0591167
…ject#121) This PR is a conglomerate of the following PRs. opensearch-project#60 opensearch-project#64 opensearch-project#65 opensearch-project#67 opensearch-project#68 opensearch-project#69 opensearch-project#70 opensearch-project#71 opensearch-project#74 opensearch-project#75 opensearch-project#76 opensearch-project#77 opensearch-project#78 opensearch-project#79 opensearch-project#82 opensearch-project#83 opensearch-project#84 opensearch-project#92 opensearch-project#94 opensearch-project#93 opensearch-project#95 kaituo#1 kaituo#2 kaituo#3 kaituo#4 kaituo#5 kaituo#6 kaituo#7 kaituo#8 kaituo#9 kaituo#10 This spreadsheet contains the mappings from files to PR number (bug fix in my AD fork and tests are not included): https://gist.github.com/kaituo/9e1592c4ac4f2f449356cb93d0591167
This PR is a conglomerate of the following PRs. #60 #64 #65 #67 #68 #69 #70 #71 #74 #75 #76 #77 #78 #79 #82 #83 #84 #92 #94 #93 #95 kaituo#1 kaituo#2 kaituo#3 kaituo#4 kaituo#5 kaituo#6 kaituo#7 kaituo#8 kaituo#9 kaituo#10 This spreadsheet contains the mappings from files to PR number (bug fix in my AD fork and tests are not included): https://gist.github.com/kaituo/9e1592c4ac4f2f449356cb93d0591167
Description
The stats API broadcasts requests to all nodes and renders node responses using toXContent. For the local node, the stats API's calls toXContent on the node response directly. For remote node, the coordinating node gets a serialized content from
ADStatsNodeResponse.writeTo, deserializes the content and renders the result using toXContent. ADStatsNodeResponse.writeTo uses StreamOutput::writeGenericValue that only recognizes built-in types (e.g., String or primitive types). It throws exceptions for types such as java.time.Instant and Entity. This PR fixes the bug by converting non-built-in types to built-in types in node stats response.
This PR also removes model id from Entity.toString as it is random across different runs and keeps only attributes. This can help locate the same node in hash ring.
This PR also removes unused fields in ADStats.
Testing done:
Issues Resolved
opensearch-project#87
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.