Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
Jul 28, 2021
Merged

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

merged 9 commits into from
Jul 28, 2021

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Jul 15, 2021 •
edited
Loading

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.

@pgomulka
[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 pgomulka marked this pull request as draft July 15, 2021 10:25
@sethmlarson sethmlarson added the Team:Clients Meta label for clients team label Jul 15, 2021
@pgomulka pgomulka marked this pull request as ready for review July 16, 2021 07:16
@elasticmachine
Copy link
Collaborator

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

@pgomulka pgomulka added the :Core/Infra/REST API REST infrastructure and utilities label Jul 16, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 16, 2021
@elasticmachine
Copy link
Collaborator

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

pgomulka
pgomulka commented Jul 16, 2021
View reviewed changes
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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pgomulka self-assigned this Jul 16, 2021
@pgomulka pgomulka added the :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. label Jul 19, 2021
@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label Jul 19, 2021
@elasticmachine
Copy link
Collaborator

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

DaveCTurner
DaveCTurner reviewed Jul 27, 2021
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of comments, looks good otherwise.

qa/full-cluster-restart/src/test/java/org/elasticsearch/upgrades/FullClusterRestartIT.java Outdated
@@ -683,7 +683,7 @@ public void testRecovery() throws Exception {
flushRequest.addParameter("wait_if_ongoing", "true");
assertOK(client().performRequest(flushRequest));
if (randomBoolean()) {
syncedFlush(index);
flush(index, randomBoolean());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
pgomulka commented Jul 28, 2021
View reviewed changes
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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

DaveCTurner
DaveCTurner approved these changes Jul 28, 2021
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
@pgomulka @DaveCTurner
Update qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/Ind…
a2a053a 
…exingIT.java

Co-authored-by: David Turner <david.turner@elastic.co>
@pgomulka @DaveCTurner
Update qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/Ind…
bf35bc4 
…exingIT.java

Co-authored-by: David Turner <david.turner@elastic.co>
@pgomulka pgomulka merged commit c96139d into elastic:master Jul 28, 2021
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Jul 30, 2021
@pgomulka @ywangd
[Rest Api Compatibility] Deprecate the use of synced flush (elastic#7…
f018dc1 
…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
@mark-vieira mark-vieira removed the v8.0.0 label Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaveCTurner DaveCTurner DaveCTurner approved these changes

Assignees

@pgomulka pgomulka

Labels
:Core/Infra/REST API REST infrastructure and utilities :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. Team:Clients Meta label for clients team Team:Core/Infra Meta label for core/infra team Team:Distributed Meta label for distributed team v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pgomulka @elasticmachine @DaveCTurner @mark-vieira @sethmlarson @elasticsearchmachine