-
Notifications
You must be signed in to change notification settings - Fork 25.3k
[ML-DataFrame] migrate to PageParams for get and stats, move PageParams into core #41851
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
Conversation
fix possible NPE in PageParams
Pinging @elastic/ml-core |
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
A few minor comments, mostly around unit tests.
.../src/test/java/org/elasticsearch/client/documentation/DataFrameTransformDocumentationIT.java
Outdated
Show resolved
Hide resolved
.../rest-high-level/src/test/java/org/elasticsearch/client/DataFrameRequestConvertersTests.java
Outdated
Show resolved
Hide resolved
.../rest-high-level/src/test/java/org/elasticsearch/client/DataFrameRequestConvertersTests.java
Outdated
Show resolved
Hide resolved
.../rest-high-level/src/test/java/org/elasticsearch/client/DataFrameRequestConvertersTests.java
Outdated
Show resolved
Hide resolved
@@ -57,11 +57,11 @@ public PageParams(@Nullable Integer from, @Nullable Integer size) { | |||
this.size = size; | |||
} | |||
|
|||
public int getFrom() { | |||
public Integer getFrom() { |
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.
FYI, I did the same thing in feature branch:
https://github.com/elastic/elasticsearch/pull/41530/files
run elasticsearch-ci/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.
…ms into core (#41851) migrate hlrc dataframe get and _stats to use PageParams, moves PageParams into core for common usage, fix possible NPE in PageParams
* elastic/master: (50 commits) Cleanup versioned deprecations in analysis (elastic#41560) Allow unknown task time in QueueResizingEsTPE (elastic#41810) SQL: Remove CircuitBreaker from parser (elastic#41835) [DOCS] Fix callouts for dataframe APIs (elastic#41904) Handle serialization exceptions during publication (elastic#41781) Correct spelling of MockLogAppender.PatternSeenEventExpectation (elastic#41893) Update TLS ciphers and protocols for JDK 11 (elastic#41808) Remove Harmful Exists Check from BlobStoreFormat (elastic#41898) Fix fractional seconds for strict_date_optional_time (elastic#41871) Remove op.name configuration setting (elastic#41445) Reject port ranges in `discovery.seed_hosts` (elastic#41404) [ML-DataFrame] migrate to PageParams for get and stats, move PageParams into core (elastic#41851) Reenable RareClusterStateIT Mapping Propagation Tests (elastic#41884) [DOCS] Rewrite `exists` query docs (elastic#41868) Revert "Mute MinimumMasterNodesIT.testThreeNodesNoMasterBlock()" [DOCS] Fix typo referring to multi search API Provide names for all artifact repositories (elastic#41857) Move InternalAggregations to Writeable (elastic#41841) Fix compilation after incorrect merge Unmute TestClustersPluginIT.testMultiNode (elastic#41340) ...
…ms into core (elastic#41851) migrate hlrc dataframe get and _stats to use PageParams, moves PageParams into core for common usage, fix possible NPE in PageParams
migrates hlrc dataframe get and _stats to use PageParams, moves PageParams into core for common usage, fix possible NPE in PageParams
Note: The NPE problem is benign because ml clients send page parameters in the body, therefore still non-issue