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

Address open TODOs for Graylog 2.2.0 #3211

Merged
merged 22 commits into from Dec 21, 2016

Conversation

Projects
None yet
2 participants
@joschi
Contributor

joschi commented Dec 15, 2016

This PR addresses some open TODOs:

  • Remove TODO about ZonedDateTime in IndexSetConfig
  • Migrate remaining index settings from config file to IndexSetConfig
  • Allow overriding analyzer in MessageResource#analyze()

@joschi joschi added this to the 2.2.0 milestone Dec 15, 2016

@joschi joschi referenced this pull request Dec 15, 2016

Closed

Support for multiple retention strategies #2880

23 of 27 tasks complete

@bernd bernd self-assigned this Dec 15, 2016

.indexAnalyzer(elasticsearchConfiguration.getAnalyzer())
.indexTemplateName(elasticsearchConfiguration.getTemplateName())
.indexOptimizationMaxNumSegments(elasticsearchConfiguration.getIndexOptimizationMaxNumSegments())
.indexOptimizationDisabled(elasticsearchConfiguration.isDisableIndexOptimization())

This comment has been minimized.

@bernd

bernd Dec 15, 2016

Member

Modifying the existing migration will break existing beta.2 setups because the default index set already exists.

Since we need a new migration anyway (see above), we should create a new one.

@JsonProperty("index_template_name")
@JsonIgnore
public String indexTemplateName() {
return "graylog-internal-" + requireNonNull(id());

This comment has been minimized.

@bernd

bernd Dec 15, 2016

Member

This and all other settings need a migration to not break existing index sets in a 2.2 setup.

We also need UI components and/or good defaults, otherwise the creation of new index sets will not work.

This comment has been minimized.

@joschi

joschi Dec 20, 2016

Contributor

This will be handled in IndexSetSummary#toIndexSetConfig().

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

I think we should handle all defaults in the @JsonCreator method in this class because that's the place where the fields can be null when coming out of the database. We should remove the @Nullable annotation from the new field methods because they shouldn't ever be null. Otherwise we have to adjust all consumers of the methods to handle the null case. (they don't do that now)

This comment has been minimized.

@joschi

joschi Dec 20, 2016

Contributor

@NotBlank
public abstract String indexAnalyzer();
@JsonProperty("index_template_name")

This comment has been minimized.

@bernd

bernd Dec 15, 2016

Member

I think we shouldn't allow the update of the index template name for an index set. This makes it harder to cleanup the template when an index set gets deleted.

This comment has been minimized.

@joschi

joschi Dec 16, 2016

Contributor

Should this be settable at all then?

This comment has been minimized.

@bernd

bernd Dec 16, 2016

Member

Yeah, I thought about this as well. The main problem is that we have to migrate the existing setting into the default index set so we need this in the database. But for new ones I would say prefix + index set id would be perfectly fine. So yeah, I guess it shouldn't be user configurable.

@joschi joschi force-pushed the index-set-todos branch 2 times, most recently from 8a5072b to 6d954e1 Dec 16, 2016

joschi added some commits Dec 15, 2016

Remove TODO about ZonedDateTime in IndexSetConfig
The issue has been resolved in #3198
Migrate remaining index settings from config file to IndexSetConfig
* elasticsearch_analyzer
* elasticsearch_template_name
* index_optimization_max_num_segments
* disable_index_optimization
Ignore unknown properties in IndexSetConfig
Deserializing used to fail because of the removed "default" field.

@joschi joschi force-pushed the index-set-todos branch from 6d954e1 to 1895cf5 Dec 20, 2016

joschi added some commits Dec 20, 2016

Use fixed index template name
The index set config ID isn't available at the time of creation.
checkState(clusterConfigService.get(DefaultIndexSetCreated.class) != null, "The default index set hasn't been created yet. This is a bug!");
// TODO: Migrate to new method once it's been merged
final IndexSetConfig indexSetConfig = indexSetService.getDefault();

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

Why do you only migrate the default index set? The others should also get the settings, IMHO. (they would be null otherwise and probably break stuff - see comment above about nullable)

This comment has been minimized.

@joschi

joschi Dec 20, 2016

Contributor

@JsonProperty("index_template_name")
@JsonIgnore
public String indexTemplateName() {
return "graylog-internal-" + requireNonNull(id());

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

I think we should handle all defaults in the @JsonCreator method in this class because that's the place where the fields can be null when coming out of the database. We should remove the @Nullable annotation from the new field methods because they shouldn't ever be null. Otherwise we have to adjust all consumers of the methods to handle the null case. (they don't do that now)

@AutoValue
@WithBeanGetter
@JsonAutoDetect
@JsonIgnoreProperties(ignoreUnknown = true)

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

Why do we need this?

This comment has been minimized.

@joschi

joschi Dec 20, 2016

Contributor

Otherwise loading the IndexSetConfig from the database failed due to the unknown default field.

}
@GET
@Timed
@ApiOperation(value = "Get relevant configuration settings and their values")
public ExposedConfiguration getRelevant() {
return ExposedConfiguration.create(configuration, esConfiguration);
return ExposedConfiguration.create(configuration);

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

This needs a note in the UPGRADING document since we are removing some data from the API endpoint.

This comment has been minimized.

@joschi

joschi Dec 20, 2016

Contributor

.retentionStrategy(retentionStrategy())
.creationDate(oldConfig.creationDate())
.build();
return IndexSetConfig.create(

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

At one point we should agree on how we do auto value object creation so we don't change it in every PR. 😉

This comment has been minimized.

@joschi

joschi Dec 20, 2016

Contributor

Reverted to builder style.

joschi added some commits Dec 20, 2016

Fix V20161116172100_DefaultIndexSetMigration
The builder for IndexSetConfig requires all settings (in contrast to the IndexSetConfig#create() method).
final String analyzer = elasticsearchConfiguration.getAnalyzer();
final IndexSetConfig updatedConfig = indexSetConfig.toBuilder()
.indexAnalyzer(analyzer)
.indexTemplateName(elasticsearchConfiguration.getTemplateName())

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

This is wrong, this needs to be unique and <index prefix>-template. (we should probably create a unique index in MongoDB)

Only the default index set needs the value from elasticsearchConfiguration.getTemplateName().

This comment has been minimized.

@joschi

joschi Dec 20, 2016

Contributor

Why does the template need to be unique?

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

Because otherwise we will overwrite the same index template for every index set. Every index set has a different wildcard template field.

Example:

http :9200/_template | jq '.[].template'
"emails_*"
"yolo_*"
"restored-archive-*"
"graylog_*"

This comment has been minimized.

@joschi

joschi Dec 20, 2016

Contributor

Ah, okay. So this is a restriction of our index set implementation. We should think about allowing to use the same index template for multiple index sets (multiple wildcards are possible).

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

Yes, that was the easiest thing to do at the moment. I would not allow using the same index template for multiple index sets until we have a proper UI to manage Elasticsearch index templates, though.

@@ -73,7 +73,7 @@ public void upgrade() {
final String analyzer = elasticsearchConfiguration.getAnalyzer();
final IndexSetConfig updatedConfig = indexSetConfig.toBuilder()
.indexAnalyzer(analyzer)
.indexTemplateName(elasticsearchConfiguration.getTemplateName())
.indexTemplateName(indexSetConfig.indexPrefix() + "-template")

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

As written earlier, the default index set needs the template name from the old ES configuration options. It might have been modified by the user and our default template for new indices would break that.

This comment has been minimized.

@joschi

joschi Dec 20, 2016

Contributor

The default template cannot be modified. It's always overwritten by Graylog.

This comment has been minimized.

@bernd

bernd Dec 20, 2016

Member

Hmm, true. So the config option was basically a no-op?

This comment has been minimized.

@joschi

joschi Dec 20, 2016

Contributor

No, it simply let users decide how to name the Graylog index template. If they needed modifications of the mapping, they could override them with a custom index template: http://docs.graylog.org/en/2.1/pages/configuration/elasticsearch.html#custom-index-mappings

joschi added some commits Dec 20, 2016

Remove @JsonIgnoreProperties from IndexSetConfig
Was "needed" due to a broken migration during development.
@bernd

bernd approved these changes Dec 21, 2016

@bernd bernd merged commit 3f94dc5 into master Dec 21, 2016

4 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 1184 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@bernd bernd deleted the index-set-todos branch Dec 21, 2016

@bernd bernd removed the ready-for-review label Dec 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment