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

Normalized prefix for rollover API #57271

Merged
merged 7 commits into from
Jun 16, 2020
Merged

Normalized prefix for rollover API #57271

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2e33171
Normalized prefix for rollover API
Gaurav614 May 28, 2020
96f381c
Merge branch 'master' into fix_rollover_bug
Gaurav614 Jun 10, 2020
3a7ecb8
Added new Test case and renamed methods
Gaurav614 Jun 10, 2020
1f68cb6
Fixed checkstyle errors
Gaurav614 Jun 12, 2020
ba69fe1
Merge branch 'master' into fix_rollover_bug
Gaurav614 Jun 12, 2020
891e0c6
Merge branch 'master' into fix_rollover_bug
elasticmachine Jun 12, 2020
39e2c85
Fix checkstyle in MetadataRolloverServiceTests
dakrone Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .../internalClusterTest/java/org/elasticsearch/action/admin/indices/rollover/RolloverIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ public void testRolloverWithIndexSettings() throws Exception {
indexDoc("test_index-2", "1", "field", "value");
flush("test_index-2");
final Settings settings = Settings.builder()
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
.put("number_of_shards", 1) // testing without "index" prefix
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer a dedicated test for this, just so that it doesn't get accidentally removed and the behavior reverted in the future, could you add a test for this? (it can basically be a copy of this test with a descriptive name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, made a separate test case for it !

.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
.build();
final RolloverResponse response = client().admin().indices().prepareRolloverIndex("test_alias")
Expand Down
1 change: 0 additions & 1 deletion ...rc/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ public class MetadataRolloverService {
private final MetadataCreateIndexService createIndexService;
private final MetadataIndexAliasesService indexAliasesService;
private final IndexNameExpressionResolver indexNameExpressionResolver;

@Inject
public MetadataRolloverService(ThreadPool threadPool,
MetadataCreateIndexService createIndexService, MetadataIndexAliasesService indexAliasesService,
Expand Down
13 changes: 9 additions & 4 deletions server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,10 +277,7 @@ public void createIndex(final CreateIndexClusterStateUpdateRequest request,

private void onlyCreateIndex(final CreateIndexClusterStateUpdateRequest request,
final ActionListener<ClusterStateUpdateResponse> listener) {
Settings.Builder updatedSettingsBuilder = Settings.builder();
Settings build = updatedSettingsBuilder.put(request.settings()).normalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX).build();
indexScopedSettings.validate(build, true); // we do validate here - index setting must be consistent
request.settings(build);
applyCreateIndexRequestInternal(request);
clusterService.submitStateUpdateTask(
"create-index [" + request.index() + "], cause [" + request.cause() + "]",
new AckedClusterStateUpdateTask<>(Priority.URGENT, request, listener) {
Expand All @@ -306,12 +303,20 @@ public void onFailure(String source, Exception e) {
});
}

private void applyCreateIndexRequestInternal(CreateIndexClusterStateUpdateRequest createIndexClusterStateRequest){
Copy link
Member

Choose a reason for hiding this comment

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

Calling this "apply" I think is a little misleading, I think normalizeRequestSettings would be a better name for this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

Settings.Builder updatedSettingsBuilder = Settings.builder();
Settings build = updatedSettingsBuilder.put(createIndexClusterStateRequest.settings()).normalizePrefix(IndexMetadata.INDEX_SETTING_PREFIX).build();
indexScopedSettings.validate(build,true);
createIndexClusterStateRequest.settings(build);
}
/**
* Handles the cluster state transition to a version that reflects the {@link CreateIndexClusterStateUpdateRequest}.
* All the requested changes are firstly validated before mutating the {@link ClusterState}.
*/
public ClusterState applyCreateIndexRequest(ClusterState currentState, CreateIndexClusterStateUpdateRequest request, boolean silent,
BiConsumer<Metadata.Builder, IndexMetadata> metadataTransformer) throws Exception {

applyCreateIndexRequestInternal(request);
logger.trace("executing IndexCreationTask for [{}] against cluster state version [{}]", request, currentState.version());

validate(request, currentState);
Expand Down
5 changes: 3 additions & 2 deletions ...st/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.elasticsearch.common.CheckedFunction;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.UUIDs;
import org.elasticsearch.common.settings.IndexScopedSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.xcontent.json.JsonXContent;
Expand Down Expand Up @@ -471,7 +472,7 @@ public void testRolloverClusterState() throws Exception {
when(mockIndexNameExpressionResolver.resolveDateMathExpression(any())).then(returnsFirstArg());

MetadataCreateIndexService createIndexService = new MetadataCreateIndexService(Settings.EMPTY,
clusterService, indicesService, allocationService, null, env, null, testThreadPool, null, Collections.emptyList(), false);
clusterService, indicesService, allocationService, null, env, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, testThreadPool, null, Collections.emptyList(), false);
MetadataIndexAliasesService indexAliasesService = new MetadataIndexAliasesService(clusterService, indicesService,
new AliasValidator(), null, xContentRegistry());
MetadataRolloverService rolloverService = new MetadataRolloverService(testThreadPool, createIndexService, indexAliasesService,
Expand Down Expand Up @@ -536,7 +537,7 @@ public void testRolloverClusterStateForDataStream() throws Exception {
when(mockIndexNameExpressionResolver.resolveDateMathExpression(any())).then(returnsFirstArg());

MetadataCreateIndexService createIndexService = new MetadataCreateIndexService(Settings.EMPTY,
clusterService, indicesService, allocationService, null, env, null, testThreadPool, null, Collections.emptyList(), false);
clusterService, indicesService, allocationService, null, env, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, testThreadPool, null, Collections.emptyList(), false);
MetadataIndexAliasesService indexAliasesService = new MetadataIndexAliasesService(clusterService, indicesService,
new AliasValidator(), null, xContentRegistry());
MetadataRolloverService rolloverService = new MetadataRolloverService(testThreadPool, createIndexService, indexAliasesService,
Expand Down