Re-deprecate xpack rollup endpoints #36451

nik9000 · 2018-12-10T19:59:55Z

Redeprecates the /_xpack/rollup endpoints in favor of /_rollup.

When we cleanup the rollup in a cluster containing 6.x nodes we need to
use /_xpack/rollup instead of /_rollup because the 6.x nodes don't
know about /_rollup. In those cases we must ignore the deprecation
warnings that the 7.0 node will return for the end point.

Closes #36044

)"" This reverts commit 61c2db5.

…d cluster (elastic#36000)"" This reverts commit 40c5445.

When we cleanup the rollup in a cluster containing 6.x nodes we need to use `/_xpack/rollup` instead of `/_rollup` because the 6.x nodes don't know about `/_rollup`. In those cases we must ignore the deprecation warnings that the 7.0 node will return for the end point. Closes elastic#36044

elasticmachine · 2018-12-10T19:59:57Z

Pinging @elastic/es-analytics-geo

nik9000 · 2018-12-10T20:04:26Z

@jasontedor I see that you made non-_xpack rollup endpoints in another PR. Would you like for me to deprecate the _xpack endpoints as part of this one?

jasontedor · 2018-12-10T20:36:38Z

Yes please, sorry for the conflicts.

nik9000 · 2018-12-10T20:38:15Z

Yes please, sorry for the conflicts.

Will do! Conflicts happen.

nik9000 · 2018-12-10T21:36:30Z

So now that I've merged this I'm not sure that I need the warnings work.... Let me check if I can drop that.

nik9000 · 2018-12-10T22:57:54Z

So now that I've merged this I'm not sure that I need the warnings work.... Let me check if I can drop that.

Yes indeed. I can just revert all of that. We don't need it now.

jasontedor · 2018-12-11T01:05:04Z

@elasticmachine test this please

nik9000 · 2018-12-11T13:35:13Z

...ugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/rest/RestDeleteRollupJobAction.java

    @Override
    public String getName() {
-        return "rollup_delete_job_action";
+        return "delete_rollup_job";


I've noticed a few of these changes that came along with my unrevert. Are these changes compatible with a mixed version cluster?

That's a good question... I'm not sure. I don't really know how REST request names are used (or if they are just for display/task purposes?)

They are used in the usage API, and ultimately intended to be surfaced in telemetry. In the long run, I want to open a change to derive the name from the class name and remove all these overrides, so in these changes I have been aligning these with that intended change, and removing "xpack" from the names in places where they exist. I am not worried about BWC during a rolling upgrade.

nik9000 · 2018-12-11T13:36:31Z

...full-cluster-restart/src/test/java/org/elasticsearch/xpack/restart/FullClusterRestartIT.java

-        final Request getRollupJobRequest = new Request("GET", "_xpack/rollup/job/" + rollupJob);
+        final Request getRollupJobRequest;
+        if (isRunningAgainstOldCluster()) {
+            getRollupJobRequest = new Request("GET", "/_xpack/rollup/job/" + rollupJob);


These are required because we can do a full cluster restart against versions that don't have the new endpoint.

nik9000 · 2018-12-11T14:16:44Z

This is ready for review so I've removed WIP.

jasontedor

Looks good. Sorry about the deprecation work/unwork; it didn't occur to me until looking at the problems that the security and ML work that we had another way to approach this that could be cleaner and avoid the difficulties we were initially trying to work around here.

nik9000 · 2018-12-11T14:48:59Z

Looks good. Sorry about the deprecation work/unwork; it didn't occur to me until looking at the problems that the security and ML work that we had another way to approach this that could be cleaner and avoid the difficulties we were initially trying to work around here.

We're going to need to do some work on deprecation when we go to remove the old APIs in master. But we should have the tools that we need for that already available then so it'll be easier.

We don't write anything and the cleanup isn't compatible.

polyfractal

LGTM, although I'm not sure about your question re: names. Left a question/comment about the rolling restart tests.

polyfractal · 2018-12-11T17:05:30Z

docs/reference/rollup/api-quickref.asciidoc


-* {ref}/rollup-get-rollup-caps.html[GET /_xpack/rollup/data/<index_pattern+++>/_rollup_caps+++]: Get Rollup Capabilities
+* {ref}/rollup-get-rollup-caps.html[GET /_rollup/data/<index_pattern+++>/_rollup_caps+++]: Get Rollup Capabilities
 * {ref}/rollup-get-rollup-index-caps.html[GET /<index_name+++>/_rollup/data/+++]: Get Rollup Index Capabilities


Heh, accidental typo becomes the correct endpoint XD

polyfractal · 2018-12-11T17:07:32Z

...ugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/rest/RestDeleteRollupJobAction.java

    @Override
    public String getName() {
-        return "rollup_delete_job_action";
+        return "delete_rollup_job";


That's a good question... I'm not sure. I don't really know how REST request names are used (or if they are just for display/task purposes?)

polyfractal · 2018-12-11T17:18:19Z

x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/RollupIDUpgradeIT.java


            // create the rollup job
-            final Request createRollupJobRequest = new Request("PUT", "/_xpack/rollup/job/rollup-id-test");
+            final Request createRollupJobRequest = new Request("PUT", "/_rollup/job/rollup-id-test");


I'm not entirely sure how rolling upgrade tests pick their versions, but couldn't we go from < 6.6 to >= 6.6, <7.0? In which case we'll need an assumeTrue() on this test, or some kind of version check on these URIs?

The rolling upgrade tests only go through the wire compatible versions so this should be safe.

jasontedor · 2018-12-12T01:01:58Z

Thank you @nik9000!

nik9000 · 2018-12-12T01:35:23Z

I'm glad our intake build went green and I could get it merged! Thanks for the review!

…

On Tue, Dec 11, 2018, 20:03 Jason Tedor ***@***.*** wrote: Thank you @nik9000 <https://github.com/nik9000>! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36451 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANLol9FSys8kD3ncomWq8GXL1ucfrFeks5u4FXDgaJpZM4ZMExL> .

nik9000 added 4 commits December 10, 2018 08:59

Revert "Revert "Deprecate X-Pack centric rollup endpoints (elastic#35962

08cfebc

)"" This reverts commit 61c2db5.

Revert "Revert "[TEST] Use deprecated form of rollup endpoint in mixe…

58bbb4c

…d cluster (elastic#36000)"" This reverts commit 40c5445.

better name

773e2fc

nik9000 added v7.0.0 :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data labels Dec 10, 2018

nik9000 added the >deprecation label Dec 10, 2018

nik9000 added 3 commits December 10, 2018 15:49

Merge branch 'master' into rollup_cleanup_deprecation

3ca74e4

word

24a55ed

Cleanup

a36531a

WIP

f72ad3e

nik9000 commented Dec 11, 2018

View reviewed changes

Remove import

11313c7

nik9000 changed the title ~~WIP: Re-deprecate xpack rollup endpoints~~ Dec 11, 2018

jasontedor approved these changes Dec 11, 2018

View reviewed changes

nik9000 added 2 commits December 11, 2018 09:49

Merge branch 'master' into rollup_cleanup_deprecation

491d2f5

Don't clean up after version check

dd72150

We don't write anything and the cleanup isn't compatible.

$polyfractal$

polyfractal approved these changes Dec 11, 2018

View reviewed changes

jasontedor added the blocker label Dec 11, 2018

nik9000 merged commit 03daad9 into elastic:master Dec 12, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

$@polyfractal$ polyfractal mentioned this pull request Jan 17, 2020

[CI]MultiVersionRepositoryAccessIT fails due rollup cleanups #51141

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Re-deprecate xpack rollup endpoints #36451

Re-deprecate xpack rollup endpoints #36451

Uh oh!

nik9000 commented Dec 10, 2018

elasticmachine commented Dec 10, 2018

nik9000 commented Dec 10, 2018

jasontedor commented Dec 10, 2018

nik9000 commented Dec 10, 2018

nik9000 commented Dec 10, 2018

nik9000 commented Dec 10, 2018

jasontedor commented Dec 11, 2018

nik9000 Dec 11, 2018

$@polyfractal$ polyfractal Dec 11, 2018

jasontedor Dec 11, 2018

nik9000 Dec 11, 2018

nik9000 commented Dec 11, 2018

jasontedor left a comment

nik9000 commented Dec 11, 2018

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal Dec 11, 2018

$@polyfractal$ polyfractal Dec 11, 2018

$@polyfractal$ polyfractal Dec 11, 2018

nik9000 Dec 11, 2018

jasontedor commented Dec 12, 2018

nik9000 commented Dec 12, 2018 via email

Re-deprecate xpack rollup endpoints #36451

Re-deprecate xpack rollup endpoints #36451

Uh oh!

Conversation

nik9000 commented Dec 10, 2018

elasticmachine commented Dec 10, 2018

nik9000 commented Dec 10, 2018

jasontedor commented Dec 10, 2018

nik9000 commented Dec 10, 2018

nik9000 commented Dec 10, 2018

nik9000 commented Dec 10, 2018

jasontedor commented Dec 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Dec 11, 2018

jasontedor left a comment

Choose a reason for hiding this comment

nik9000 commented Dec 11, 2018

polyfractal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor commented Dec 12, 2018

nik9000 commented Dec 12, 2018 via email

$@polyfractal$ polyfractal left a comment