You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/11/07 22:32:20 UTC
[GitHub] [skywalking] toffentoffen commented on a diff in pull request #9914: Elasticsearch storage: support specify the settings `(number_of_shards/number_of_replicas)` for each index individually.
toffentoffen commented on code in PR #9914:
URL: https://github.com/apache/skywalking/pull/9914#discussion_r1015963081
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -111,28 +123,36 @@ private void createNormalTable(Model model) throws StorageException {
ElasticSearchClient esClient = (ElasticSearchClient) client;
String tableName = IndexController.INSTANCE.getTableName(model);
Mappings mapping = createMapping(model);
+ Map<String, Object> settings = createSetting(model);
if (!esClient.isExistsIndex(tableName)) {
- Map<String, Object> settings = createSetting(model);
boolean isAcknowledged = esClient.createIndex(tableName, mapping, settings);
log.info("create {} index finished, isAcknowledged: {}", tableName, isAcknowledged);
if (!isAcknowledged) {
- throw new StorageException("create " + tableName + " index failure, ");
+ throw new StorageException("create " + tableName + " index failure");
}
} else {
Mappings historyMapping = esClient.getIndex(tableName)
.map(Index::getMappings)
.orElseGet(Mappings::new);
- structures.putStructure(tableName, mapping);
- Mappings appendMapping = structures.diffStructure(tableName, historyMapping);
+ structures.putStructure(tableName, mapping, settings);
+ Mappings appendMapping = structures.diffMappings(tableName, historyMapping);
+ //update mapping
if (appendMapping.getProperties() != null && !appendMapping.getProperties().isEmpty()) {
boolean isAcknowledged = esClient.updateIndexMapping(tableName, appendMapping);
- log.info("update {} index finished, isAcknowledged: {}, append mappings: {}", tableName,
+ log.info("update {} index mapping finished, isAcknowledged: {}, append mapping: {}", tableName,
isAcknowledged, appendMapping
);
if (!isAcknowledged) {
- throw new StorageException("update " + tableName + " index failure");
+ throw new StorageException("update " + tableName + " index mapping failure");
}
}
+ //needs to update settings
+ if (!structures.compareIndexSetting(tableName, settings)) {
+ log.warn(
+ "index {} settings configuration has been updated to {}, please remove it before OAP starts",
+ tableName, settings
+ );
+ }
Review Comment:
Under which scenario, which type of setting, causes that the index needs to be removed? I guess that if specific number of shards or replicas are specified for a concrete index, this warning will be displayed isn't it?
I not sure if we should display it when only shards or replicas have changed:
1. User could be confused, as the shards and replica changes only apply for new indices after the changes are applied to the index template as per https://github.com/apache/skywalking/pull/9914/files#diff-7c42c99e33896647d67385263076ec5178df8e57d072e760895ed7ca9f5d1c20R199
2. Maybe we should display an info message instead if only shards and replica number have changed, as those changes can never be applied because ES doesn't allow to change the number of shards and replicas for and existing index. Moreover, I think those changes are never applied to an existing index, that's the reason why I would suggest to not show a warning message.
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/IndexStructures.java:
##########
@@ -146,4 +172,18 @@ private Map<String, Object> diffFields(Fields fields) {
));
}
}
+
+ /**
+ * The index settings structure which only include needs to compare and update fields
+ */
+ @EqualsAndHashCode
+ public static class UpdatableIndexSettings {
+ private final int replicas;
+ private final int shards;
+
+ public UpdatableIndexSettings(Map<String, Object> indexSettings) {
+ this.replicas = Integer.parseInt((String) indexSettings.getOrDefault("number_of_replicas", "0"));
+ this.shards = Integer.parseInt((String) indexSettings.getOrDefault("number_of_shards", "0"));
Review Comment:
I am fully aware how it works globally, but I have some doubts.
1. Is it mandatory to set always both properties for a given index in the json to specify the concrete number of replicas and shards?
2. If only replicas is provided for instance, what will be the number of shards 0? Or the value specified by `SW_STORAGE_ES_INDEX_SHARDS_NUMBER` if provided or the default value of `SW_STORAGE_ES_INDEX_SHARDS_NUMBER` if absent?
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -183,13 +214,14 @@ private void createTimeSeriesTable(Model model) throws StorageException {
protected Map<String, Object> createSetting(Model model) throws StorageException {
Map<String, Object> setting = new HashMap<>();
-
- setting.put("index.number_of_replicas", model.isSuperDataset()
- ? config.getSuperDatasetIndexReplicasNumber()
- : config.getIndexReplicasNumber());
- setting.put("index.number_of_shards", model.isSuperDataset()
- ? config.getIndexShardsNumber() * config.getSuperDatasetIndexShardsFactor()
- : config.getIndexShardsNumber());
+ Map<String, Object> indexSettings = new HashMap<>();
+ setting.put("index", indexSettings);
+ indexSettings.put("number_of_replicas", model.isSuperDataset()
+ ? Integer.toString(config.getSuperDatasetIndexReplicasNumber())
+ : Integer.toString(config.getIndexReplicasNumber()));
+ indexSettings.put("number_of_shards", model.isSuperDataset()
+ ? Integer.toString(config.getIndexShardsNumber() * config.getSuperDatasetIndexShardsFactor())
Review Comment:
I would suggest that we should not multiply the number of shards by the shards factor if a concrete number of shards is configure for a given index. Otherwise, the user might be confused. If the user says I want X number of shards for zipkin index, X number of shards should be used. Otherwise, when the users checks the number of shards it will see X*Shards_Factor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org