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.

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

Resiliency: Be more conservative if index.version.created is not set #8018

Merged
merged 1 commit into from Oct 14, 2014

Conversation

Projects
None yet
5 participants
@s1monw
Copy link
Contributor

commented Oct 8, 2014

Today we use the current version which might enable features that are
not supported. We should use the minimal known version rather than
defaulting to the current.

@bleskes

This comment has been minimized.

Copy link
Member

commented Oct 8, 2014

LGTM, the only concern in have is that we do similar checks in other place is the code , searching for usage of SETTING_VERSION_CREATED shows them. I think we need to replace them for this to have proper effect.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2014

What about throwing an exception if it is not set? Things would very likely go wrong anyway if it is not set?

@bleskes

This comment has been minimized.

Copy link
Member

commented Oct 8, 2014

@jpountz we started setting it on indices as of 0.19. This is an index meta data so there is no upgrade path. Not sure what to do with indices that were created with 0.18 and long since upgraded. Officially we are still supposed to be able to read them? Maybe it should be part of the upgrade API that once we are guaranteed that all segments of an index are on the current lucene version, we can update this setting. Not sure what can of worms that will open though. We can consider dropping this with 2.0 but we should let people know and offer them a way out.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2014

LGTM, the only concern in have is that we do similar checks in other place is the code , searching for usage of SETTING_VERSION_CREATED shows them. I think we need to replace them for this to have proper effect.

I agree there should be only one way to get this maybe we can just use the Version utils everywhere?

What about throwing an exception if it is not set? Things would very likely go wrong anyway if it is not set?

I kind of agree - I wonder what would happen if we missed something here? I mean there is no real upgradepath if shit hits the fan?

@jpountz we started setting it on indices as of 0.19. This is an index meta data so there is no upgrade path. Not sure what to do with indices that were created with 0.18 and long since upgraded. Officially we are still supposed to be able to read them? Maybe it should be part of the upgrade API that once we are guaranteed that all segments of an index are on the current lucene version, we can update this setting. Not sure what can of worms that will open though. We can consider dropping this with 2.0 but we should let people know and offer them a way out.

that is actually not true - there is an upgrade path in LocalGatewayMetaState:625

@bleskes

This comment has been minimized.

Copy link
Member

commented Oct 8, 2014

I agree there should be only one way to get this maybe we can just use the Version utils everywhere?
+1

that is actually not true - there is an upgrade path in LocalGatewayMetaState:625

Fair enough :)

I kind of agree - I wonder what would happen if we missed something here? I mean there is no real upgradepath if shit hits the fan?

I wonder how bad it is to run with the wrong version (i.e., too old in this case). Wouldn't that cause way more subtle exceptions? I see things around field data and analysis.

If we are concerned about this maybe we should have a work around cluster level settings which assigns a version instead of throwing an exception. We can potentially only do it in 1.x.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2014

I agree though we should really throw an exception here :)

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2014

I wonder how bad it is to run with the wrong version

In case doc values are enabled, this can be pretty bad as eg. we recently switched from binary doc values to sorted numerics for numeric fields and lucene would refuse to index a document that has a doc value type that is different from what is currently indexed.

I agree though we should really throw an exception here :)

+1 on an exception. It would be much better than digging an issue for hours before noticing that the cause was that the version was not set.

@s1monw s1monw force-pushed the s1monw:created_version branch 2 times, most recently Oct 14, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2014

ok guys I changed this to barf if the setting is not present and I am surprised how many failures I got.... Yet I think this is the safest option to be honest!

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2014

LGTM

I agree this is safer, I like this option better!

@bleskes

This comment has been minimized.

Copy link
Member

commented Oct 14, 2014

+1

[CORE] Be more explicit if index.version.created is not set
Today we use the current version which might enable features that are
not supported. We should throw an exception if this setting is not
present for any index.

Closes #8018

@s1monw s1monw force-pushed the s1monw:created_version branch to ac4b39b Oct 14, 2014

@s1monw s1monw merged commit ac4b39b into elastic:master Oct 14, 2014

s1monw added a commit to s1monw/elasticsearch that referenced this pull request Oct 14, 2014

[CORE] Be more explicit if index.version.created is not set
Today we use the current version which might enable features that are
not supported. We should throw an exception if this setting is not
present for any index.

Closes elastic#8018

s1monw added a commit to s1monw/elasticsearch that referenced this pull request Oct 14, 2014

[CORE] Be more explicit if index.version.created is not set
Today we use the current version which might enable features that are
not supported. We should throw an exception if this setting is not
present for any index.

Closes elastic#8018

@s1monw s1monw removed the v1.3.5 label Oct 14, 2014

@s1monw s1monw deleted the s1monw:created_version branch Oct 14, 2014

@dadoonet

This comment has been minimized.

Copy link
Member

commented Oct 15, 2014

This patch breaks some plugins because of a signature change for AnalysisService(Index) which now requires Settings: `AnalysisService(Index, Settings)

It means that we will need to release analysis plugins for elasticsearch 1.4.0 in addition to 1.4.0.Beta1.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2014

wait, the AnalysisService(Index, Settings) ctor is test-only why do they depend on this?

dadoonet added a commit to elastic/elasticsearch-mapper-attachments that referenced this pull request Oct 15, 2014

Tests: AnalysisService constructor signature change
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #87.

(cherry picked from commit b3b0d34)

dadoonet added a commit to elastic/elasticsearch-mapper-attachments that referenced this pull request Oct 15, 2014

Tests: AnalysisService constructor signature change
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #87.

dadoonet added a commit to elastic/elasticsearch-mapper-attachments that referenced this pull request Oct 15, 2014

Tests: AnalysisService constructor signature change
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #87.

(cherry picked from commit b3b0d34)

dadoonet added a commit to elastic/elasticsearch-analysis-phonetic that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #33.

dadoonet added a commit to elastic/elasticsearch-analysis-phonetic that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #33.

(cherry picked from commit cb869cd)

dadoonet added a commit to elastic/elasticsearch-analysis-phonetic that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #33.

(cherry picked from commit cb869cd)

dadoonet added a commit to elastic/elasticsearch-analysis-stempel that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #33.

dadoonet added a commit to elastic/elasticsearch-analysis-stempel that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #33.

dadoonet added a commit to elastic/elasticsearch-analysis-stempel that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #33.
@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2014

ok it seems like not a breaking change for the plugins

dadoonet added a commit to elastic/elasticsearch-analysis-kuromoji that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #47.

(cherry picked from commit 3a90982)

dadoonet added a commit to elastic/elasticsearch-analysis-kuromoji that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #47.

(cherry picked from commit 3a90982)

dadoonet added a commit to elastic/elasticsearch-analysis-kuromoji that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #47.

dadoonet added a commit to elastic/elasticsearch-analysis-smartcn that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #31.

(cherry picked from commit 3eeb827)

dadoonet added a commit to elastic/elasticsearch-analysis-smartcn that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #31.

dadoonet added a commit to elastic/elasticsearch-analysis-smartcn that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #31.

(cherry picked from commit 3eeb827)

dadoonet added a commit to elastic/elasticsearch-analysis-icu that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #41.

(cherry picked from commit 75b800f)

dadoonet added a commit to elastic/elasticsearch-analysis-icu that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #41.

dadoonet added a commit to elastic/elasticsearch-analysis-icu that referenced this pull request Oct 15, 2014

Tests: index.version.created must be set
Due to this [change](elastic/elasticsearch#8018), we need to fix our tests for elasticsearch 1.4.0 and above.

Closes #41.

(cherry picked from commit 75b800f)

@clintongormley clintongormley changed the title [CORE] Be more conservative if index.version.created is not set Resiliency: Be more conservative if index.version.created is not set Nov 3, 2014

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

[CORE] Be more explicit if index.version.created is not set
Today we use the current version which might enable features that are
not supported. We should throw an exception if this setting is not
present for any index.

Closes elastic#8018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.