[Rest Api Compatibility] Deprecate the use of synced flush by pgomulka · Pull Request #75372 · elastic/elasticsearch

pgomulka · 2021-07-15T10:25:02Z

synced flush is going to be replaced by flush. This commit allows to synced_flush api only in v7 compatibility mode.
Worth noting - sync_id is gone and won't be available in v7 responses from indices.stats

relates removal pr #50882
relates #51816

Have you signed the contributor license agreement?
Have you followed the contributor guidelines?
If submitting code, have you built your formula locally prior to submission with gradle check?
If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
If submitting code, have you checked that your submission is for an OS and architecture that we support?
If you are submitting this code for a class then read our policy for that.

synced flush is going to be replaced by flush. This commit deprecates the synced flush and is meant to be backported to 7.x relates elastic#50882 relates elastic#51816

elasticmachine · 2021-07-16T07:16:35Z

Pinging @elastic/clients-team (Team:Clients)

elasticmachine · 2021-07-16T07:17:11Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

pgomulka · 2021-07-16T07:38:03Z

qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/IndexingIT.java

-            List<String> warningMsg = List.of("Synced flush was removed and a normal flush was performed instead. " +
-                "This transition will be removed in a future version.");
-            request.setOptions(RequestOptions.DEFAULT.toBuilder().setWarningsHandler(warnings -> warnings.equals(warningMsg) == false));
+            final String v7MediaType = XContentType.VND_JSON.toParsedMediaType()


I am not sure we would like to use synced flush here. Since it was deprecated in 7.x and is only available under rest compatibility in 8 I guess we should just use flush
WDYT?

The whole test is called testSyncedFlushTransition and starts with assertTrue("bwc version is on 7.x", nodes.getBWCVersion().before(Version.V_8_0_0)); so I think it's right to still use synced-flush here. We could reasonably duplicate this test as one which just uses a regular flush and will continue to work once 7.x is no more.

elasticmachine · 2021-07-19T09:29:08Z

Pinging @elastic/es-distributed (Team:Distributed)

DaveCTurner

I left a couple of comments, looks good otherwise.

DaveCTurner · 2021-07-27T11:20:34Z

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

            assertOK(client().performRequest(flushRequest));
            if (randomBoolean()) {
-                syncedFlush(index);
+                flush(index, randomBoolean());


I think we can just drop this, we already just force-flushed.

DaveCTurner · 2021-07-27T11:26:06Z

qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/IndexingIT.java

-            List<String> warningMsg = List.of("Synced flush was removed and a normal flush was performed instead. " +
-                "This transition will be removed in a future version.");
-            request.setOptions(RequestOptions.DEFAULT.toBuilder().setWarningsHandler(warnings -> warnings.equals(warningMsg) == false));
+            final String v7MediaType = XContentType.VND_JSON.toParsedMediaType()


The whole test is called testSyncedFlushTransition and starts with assertTrue("bwc version is on 7.x", nodes.getBWCVersion().before(Version.V_8_0_0)); so I think it's right to still use synced-flush here. We could reasonably duplicate this test as one which just uses a regular flush and will continue to work once 7.x is no more.

pgomulka · 2021-07-28T10:04:54Z

qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/IndexingIT.java

+                Map<String, Object> result = ObjectPath.createFromResponse(oldNodeClient.performRequest(request)).evaluate("_shards");
+                assertThat(result.get("total"), equalTo(totalShards));
+                assertThat(result.get("successful"), equalTo(totalShards));
+                assertThat(result.get("failed"), equalTo(0));


I was wondering, how is _flush smarter then _flush/_synced that it won't fail on old nodes?

I'm not sure I understand. The _flush API is ancient, it's fully supported on every version in scope here.

DaveCTurner

I suggested removing a couple of remaining mentions of synced in the new test, otherwise LGTM. No need for another review.

qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/IndexingIT.java

…exingIT.java Co-authored-by: David Turner <david.turner@elastic.co>

…5372) synced flush is going to be replaced by flush. This commit allows to synced_flush api only in v7 compatibility mode. Worth noting - sync_id is gone and won't be available in v7 responses from indices.stats relates removal pr elastic#50882 relates elastic#51816

[Rest Api Compatibility] Deprecate the use of synced flush

cbec51a

synced flush is going to be replaced by flush. This commit deprecates the synced flush and is meant to be backported to 7.x relates elastic#50882 relates elastic#51816

pgomulka marked this pull request as draft July 15, 2021 10:25

elasticsearchmachine added the v8.0.0 label Jul 15, 2021

pgomulka added 3 commits July 15, 2021 13:41

test transformations

75c8722

comment and bwc test

a1b0754

remove the use of synced flush

0260c56

sethmlarson added the Team:Clients Meta label for clients team label Jul 15, 2021

pgomulka marked this pull request as ready for review July 16, 2021 07:16

pgomulka added the :Core/Infra/REST API REST infrastructure and utilities label Jul 16, 2021

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 16, 2021

pgomulka requested a review from DaveCTurner July 16, 2021 07:17

remove comment and remove changes related to voting config exclusions

bad7fa0

pgomulka commented Jul 16, 2021

View reviewed changes

pgomulka self-assigned this Jul 16, 2021

pgomulka added the :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. label Jul 19, 2021

elasticmachine added the Team:Distributed Meta label for distributed team. label Jul 19, 2021

DaveCTurner reviewed Jul 27, 2021

View reviewed changes

pgomulka added 2 commits July 27, 2021 17:26

duplicate flush_sync using flush and remove unnecesary flush call

2fa23f7

Merge remote-tracking branch 'upstream/master' into compat/synced_flush

a27d9bc

pgomulka commented Jul 28, 2021

View reviewed changes

pgomulka requested a review from DaveCTurner July 28, 2021 10:05

DaveCTurner approved these changes Jul 28, 2021

View reviewed changes

qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/IndexingIT.java Outdated Show resolved Hide resolved

qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/IndexingIT.java Outdated Show resolved Hide resolved

pgomulka and others added 2 commits July 28, 2021 12:30

Update qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/Ind…

a2a053a

…exingIT.java Co-authored-by: David Turner <david.turner@elastic.co>

Update qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/Ind…

bf35bc4

…exingIT.java Co-authored-by: David Turner <david.turner@elastic.co>

pgomulka merged commit c96139d into elastic:master Jul 28, 2021

mark-vieira removed the v8.0.0 label Aug 4, 2021

mark-vieira added the v8.0.0-alpha1 label Aug 4, 2021

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rest Api Compatibility] Deprecate the use of synced flush#75372

[Rest Api Compatibility] Deprecate the use of synced flush#75372
pgomulka merged 9 commits intoelastic:masterfrom
pgomulka:compat/synced_flush

pgomulka commented Jul 15, 2021 •

edited

Loading

Uh oh!

elasticmachine commented Jul 16, 2021

Uh oh!

elasticmachine commented Jul 16, 2021

Uh oh!

pgomulka Jul 16, 2021

Uh oh!

DaveCTurner Jul 27, 2021

Uh oh!

elasticmachine commented Jul 19, 2021

Uh oh!

DaveCTurner left a comment

Uh oh!

DaveCTurner Jul 27, 2021

Uh oh!

DaveCTurner Jul 27, 2021

Uh oh!

pgomulka Jul 28, 2021

Uh oh!

DaveCTurner Jul 28, 2021

Uh oh!

DaveCTurner left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

pgomulka commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 16, 2021

Uh oh!

elasticmachine commented Jul 16, 2021

Uh oh!

pgomulka Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 19, 2021

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

pgomulka Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pgomulka commented Jul 15, 2021 •

edited

Loading