Skip to content

fix a few versionAdded values in ElasticsearchExceptions#37877

Merged
talevy merged 4 commits intoelastic:masterfrom
talevy:versionAdded
Jan 31, 2019
Merged

fix a few versionAdded values in ElasticsearchExceptions#37877
talevy merged 4 commits intoelastic:masterfrom
talevy:versionAdded

Conversation

@talevy
Copy link
Copy Markdown
Contributor

@talevy talevy commented Jan 25, 2019

TooManyBucketsException was introduced in v6.2
and SnapshotInProgressException was introduced in v6.7

@talevy
fix a few versionAdded values in ElasticsearchExceptions
8f9caa5 
TooManyBucketsException was introduced in v6.2
and SnapshotInProgressException was introduced in v6.7
@talevy talevy added the :Core/Infra/Core Core issues without another label label Jan 25, 2019
@talevy talevy requested a review from DaveCTurner January 25, 2019 17:17
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Jan 25, 2019

Pinging @elastic/es-core-infra

@talevy
Copy link
Copy Markdown
Contributor Author

talevy commented Jan 25, 2019

I am not familiar with the semantics of versionAdded. I haven't seen any issues
come up with TooManyBucketsException, so I am not sure what guarantees are
needed to be verified in tests for this change.

@DaveCTurner
Copy link
Copy Markdown
Member

DaveCTurner commented Jan 28, 2019

It's used here, to decide whether to serialise the exception "properly" or whether to use a NotSerializableExceptionWrapper based on the remote node's version:

elasticsearch/server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java

Lines 948 to 952 in 787acb1

if (throwable instanceof ElasticsearchException && ElasticsearchException.isRegistered(throwable.getClass(), version)) {
ex = (ElasticsearchException) throwable;
} else {
ex = new NotSerializableExceptionWrapper(throwable);
}

This means that, with today's master, a 7.0 node that wants to throw a SnapshotInProgressException at a 6.7 node will fall back to a NotSerializableExceptionWrapper, so that this line (in 6.x) doesn't trigger in a mixed cluster:

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/AsyncRetryDuringSnapshotActionStep.java

Line 73 in c8905da

if (e instanceof SnapshotInProgressException) {

Unfortunately you can only really catch this with a mixed-cluster REST test.

I think we should consider adding an assertion that checks that once we've deserialised an exception from a remote node we believe that we could serialise it back to that node based on its version. I think that'd help catch this kind of asymmetry.

@talevy
Copy link
Copy Markdown
Contributor Author

talevy commented Jan 28, 2019

Cool, thanks. will look into adding this

@talevy
Copy link
Copy Markdown
Contributor Author

talevy commented Jan 29, 2019

@DaveCTurner I decided just to add unit tests for the serialization.

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/AsyncRetryDuringSnapshotActionStep.java

Line 73 in c8905da

if (e instanceof SnapshotInProgressException) {

Not sure if the above line is really testable since all the actions in ILM are done on the same node. So either the master node requested to delete or close an index on an upgraded 6.7 node, in which case, it would pick this up with no issue.

This does make me realize that running these ILM steps against 6.6 nodes would cause things to fail. That being said, we are in the process of writing documentation basically saying that mixed-clusters is not supported

let me know what you think!

@talevy
Copy link
Copy Markdown
Contributor Author

talevy commented Jan 29, 2019

run elasticsearch-ci/default-distro

@talevy
Copy link
Copy Markdown
Contributor Author

talevy commented Jan 30, 2019

run elasticsearch-ci/1

@talevy
Copy link
Copy Markdown
Contributor Author

talevy commented Jan 31, 2019

hey @DaveCTurner, mind taking another look?

DaveCTurner
DaveCTurner approved these changes Jan 31, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@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.

Sure, LGTM, sorry for the delay.

@talevy talevy merged commit 9923f0f into elastic:master Jan 31, 2019
@talevy talevy deleted the versionAdded branch January 31, 2019 16:28
@talevy
Copy link
Copy Markdown
Contributor Author

talevy commented Jan 31, 2019

thanks David!

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 31, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into retention-leases-v…
2d3772c 
…ersion

* elastic/master:
  Do not set up NodeAndClusterIdStateListener in test (elastic#38110)
  ML: better handle task state race condition (elastic#38040)
  Soft-deletes policy should always fetch latest leases (elastic#37940)
  Handle scheduler exceptions (elastic#38014)
  Minor logging improvements (elastic#38084)
  Fix Painless void return bug (elastic#38046)
  Update PutFollowAction serialization post-backport (elastic#37989)
  fix a few versionAdded values in ElasticsearchExceptions (elastic#37877)
  Reenable BWC tests after backport of elastic#37899 (elastic#38093)
  Mute failing test
  Mute failing test
  Fail start on obsolete indices documentation (elastic#37786)
  SQL: Implement FIRST/LAST aggregate functions (elastic#37936)
  Un-mute NoMasterNodeIT.testNoMasterActionsWriteMasterBlock
  remove unused parser fields in RemoteResponseParsers
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

No one assigned

Labels

:Core/Infra/Core Core issues without another label >enhancement v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@talevy @elasticmachine @DaveCTurner @colings86