wu-sheng commented on a change in pull request #6249:
URL: https://github.com/apache/skywalking/pull/6249#discussion_r563307896
##########
File path:
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/StorageModuleElasticsearchConfig.java
##########
@@ -20,6 +20,7 @@
import lombok.Getter;
import lombok.Setter;
+import org.apache.skywalking.oap.server.core.storage.annotation.Column;
Review comment:
This is illegal import.
##########
File path:
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/storage/annotation/Column.java
##########
@@ -87,6 +86,27 @@
*/
ValueDataType dataType() default ValueDataType.NOT_VALUE;
+ /**
+ * The storage analyzer mode.
+ *
+ * @since 8.4.0
+ */
+ AnalyzerType analyzer() default AnalyzerType.OAP_ANALYZER;
+
+ /**
+ * The analyzer declares the text analysis mode.
+ */
+ enum AnalyzerType {
Review comment:
As an enum type you should put more information in the type, rather than
separating them in the codes.
##########
File path:
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/storage/annotation/Column.java
##########
@@ -87,6 +86,27 @@
*/
ValueDataType dataType() default ValueDataType.NOT_VALUE;
+ /**
+ * The storage analyzer mode.
+ *
+ * @since 8.4.0
+ */
+ AnalyzerType analyzer() default AnalyzerType.OAP_ANALYZER;
+
+ /**
+ * The analyzer declares the text analysis mode.
+ */
+ enum AnalyzerType {
Review comment:
You should add `getSettings` method to return the settings.
##########
File path:
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -113,14 +115,32 @@ protected void createTable(Model model) throws
StorageException {
setting.put("index.refresh_interval", model.isRecord()
? TimeValue.timeValueSeconds(10).toString()
:
TimeValue.timeValueSeconds(config.getFlushInterval()).toString());
- setting.put("analysis.analyzer.oap_analyzer.type", "stop");
+ setting.put("analysis", getAnalyzerSetting(model.getAnalyzer()));
Review comment:
I think we should separate settings based on model's columns. Then we
don't need `combine` anymore. Also, I think it should be easier to understand.
##########
File path:
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/storage/annotation/Column.java
##########
@@ -87,6 +86,27 @@
*/
ValueDataType dataType() default ValueDataType.NOT_VALUE;
+ /**
+ * The storage analyzer mode.
+ *
+ * @since 8.4.0
+ */
+ AnalyzerType analyzer() default AnalyzerType.OAP_ANALYZER;
+
+ /**
+ * The analyzer declares the text analysis mode.
+ */
+ enum AnalyzerType {
Review comment:
As an enum, I think a method is enough. We would use the interface to
anywhere else.
##########
File path:
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -113,14 +115,32 @@ protected void createTable(Model model) throws
StorageException {
setting.put("index.refresh_interval", model.isRecord()
? TimeValue.timeValueSeconds(10).toString()
:
TimeValue.timeValueSeconds(config.getFlushInterval()).toString());
- setting.put("analysis.analyzer.oap_analyzer.type", "stop");
+ setting.put("analysis", getAnalyzerSetting(model.getAnalyzer()));
Review comment:
My point is, when do you need `oap_analyzer` and `oap_log_analyzer` at
the same time?
##########
File path:
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/StorageModuleElasticsearchConfig.java
##########
@@ -20,6 +20,7 @@
import lombok.Getter;
import lombok.Setter;
+import org.apache.skywalking.oap.server.core.storage.annotation.Column;
Review comment:
I think you will see `maven` failure. You need to use the full name in
the `@link`.
##########
File path:
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -113,14 +115,32 @@ protected void createTable(Model model) throws
StorageException {
setting.put("index.refresh_interval", model.isRecord()
? TimeValue.timeValueSeconds(10).toString()
:
TimeValue.timeValueSeconds(config.getFlushInterval()).toString());
- setting.put("analysis.analyzer.oap_analyzer.type", "stop");
+ setting.put("analysis", getAnalyzerSetting(model.getAnalyzer()));
Review comment:
You have changed `Column` annotation, doesn't you? In there you have
known the analyzer, and here, you have `Model model`, which provided the
information you need.
There are 2 ways to enhance the analyzer mechanism
1. What you do right now, need to support combine.
2. Every index has one analyzer based on Column declaration. No combine
needed.
My question is, do you have any case to support 2 analyzer(s) for one index.
##########
File path:
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/AnalyzerSetting.java
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.server.storage.plugin.elasticsearch.base;
+
+import com.google.gson.Gson;
+import com.google.gson.annotations.SerializedName;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.storage.StorageException;
+import org.apache.skywalking.oap.server.core.storage.annotation.Column;
+import
org.apache.skywalking.oap.server.storage.plugin.elasticsearch.StorageModuleElasticsearchConfig;
+
+@Getter
+@Setter
+@Slf4j
+public class AnalyzerSetting {
+ /**
+ * A built-in or customised tokenizer.
+ */
+ private Map<String, Object> tokenizer = new HashMap<>();
+ /**
+ * An optional array of built-in or customised character filters.
+ */
+ @SerializedName("char_filter")
+ private Map<String, Object> charFilter = new HashMap<>();
+ /**
+ * An optional array of built-in or customised token filters.
+ */
+ private Map<String, Object> filter = new HashMap<>();
+ /**
+ * The custom analyzers.
+ */
+ private Map<String, Object> analyzer = new HashMap<>();
+
+ public void combine(AnalyzerSetting analyzerSetting) {
+ this.analyzer.putAll(analyzerSetting.getAnalyzer());
+ this.tokenizer.putAll(analyzerSetting.tokenizer);
+ this.filter.putAll(analyzerSetting.filter);
+ this.charFilter.putAll(analyzerSetting.charFilter);
+ }
+
+ @Override
+ public boolean equals(final Object o) {
+ if (this == o)
+ return true;
+ if (!(o instanceof AnalyzerSetting))
+ return false;
+ final AnalyzerSetting that = (AnalyzerSetting) o;
+ return getTokenizer().equals(that.getTokenizer()) &&
+ getCharFilter().equals(that.getCharFilter()) &&
+ getFilter().equals(that.getFilter()) &&
+ getAnalyzer().equals(that.getAnalyzer());
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(getTokenizer(), getCharFilter(), getFilter(),
getAnalyzer());
+ }
+
+ public enum Generator {
+ OAP_ANALYZER_SETTING_GENERATOR(
+ Column.AnalyzerType.OAP_ANALYZER,
+ config -> new Gson().fromJson(config.getOapAnalyzer(),
AnalyzerSetting.class)
+ ),
+ OAP_LOG_ANALYZER_SETTING_GENERATOR(
+ Column.AnalyzerType.OAP_LOG_ANALYZER,
+ config -> new Gson().fromJson(config.getOapLogAnalyzer(),
AnalyzerSetting.class)
+ );
+
+ private final Column.AnalyzerType type;
+ private final GenerateAnalyzerSettingFunc func;
+
+ Generator(final Column.AnalyzerType type,
+ final GenerateAnalyzerSettingFunc func) {
+ this.type = type;
+ this.func = func;
+ }
+
+ public GenerateAnalyzerSettingFunc GetGenerateFunc() {
Review comment:
Why these methods have first letter in upper case?
##########
File path:
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/storage/model/Model.java
##########
@@ -37,14 +39,16 @@
private final boolean record;
private final boolean superDataset;
private final boolean isTimeSeries;
+ private final Set<Column.AnalyzerType> analyzer;
public Model(final String name,
final List<ModelColumn> columns,
final List<ExtraQueryIndex> extraQueryIndices,
final int scopeId,
final DownSampling downsampling,
final boolean record,
- final boolean superDataset) {
+ final boolean superDataset,
+ final Set<Column.AnalyzerType> analyzer) {
Review comment:
`List<ModelColumn> columns` should include the analyzer(s) already, we
don't need to add this new parameter.
##########
File path:
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/storage/model/ModelColumn.java
##########
@@ -30,19 +31,21 @@
private final boolean matchQuery;
private final boolean storageOnly;
private final int length;
+ private final Column.AnalyzerType analyzer;
public ModelColumn(ColumnName columnName,
Class<?> type,
Type genericType,
boolean matchQuery,
boolean storageOnly,
boolean isValue,
- int length) {
+ int length, final Column.AnalyzerType analyzer) {
Review comment:
Please use a new line to follow the format. And make all parameters `as
final` or none. Don't mix the style
##########
File path:
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/AnalyzerSetting.java
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.server.storage.plugin.elasticsearch.base;
+
+import com.google.gson.Gson;
+import com.google.gson.annotations.SerializedName;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.storage.StorageException;
+import org.apache.skywalking.oap.server.core.storage.annotation.Column;
+import
org.apache.skywalking.oap.server.storage.plugin.elasticsearch.StorageModuleElasticsearchConfig;
+
+@Getter
+@Setter
+@Slf4j
+public class AnalyzerSetting {
+ /**
+ * A built-in or customised tokenizer.
+ */
+ private Map<String, Object> tokenizer = new HashMap<>();
+ /**
+ * An optional array of built-in or customised character filters.
+ */
+ @SerializedName("char_filter")
+ private Map<String, Object> charFilter = new HashMap<>();
+ /**
+ * An optional array of built-in or customised token filters.
+ */
+ private Map<String, Object> filter = new HashMap<>();
+ /**
+ * The custom analyzers.
+ */
+ private Map<String, Object> analyzer = new HashMap<>();
+
+ public void combine(AnalyzerSetting analyzerSetting) {
+ this.analyzer.putAll(analyzerSetting.getAnalyzer());
+ this.tokenizer.putAll(analyzerSetting.tokenizer);
+ this.filter.putAll(analyzerSetting.filter);
+ this.charFilter.putAll(analyzerSetting.charFilter);
+ }
+
+ @Override
+ public boolean equals(final Object o) {
+ if (this == o)
+ return true;
+ if (!(o instanceof AnalyzerSetting))
+ return false;
+ final AnalyzerSetting that = (AnalyzerSetting) o;
+ return getTokenizer().equals(that.getTokenizer()) &&
+ getCharFilter().equals(that.getCharFilter()) &&
+ getFilter().equals(that.getFilter()) &&
+ getAnalyzer().equals(that.getAnalyzer());
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(getTokenizer(), getCharFilter(), getFilter(),
getAnalyzer());
+ }
+
+ public enum Generator {
+ OAP_ANALYZER_SETTING_GENERATOR(
+ Column.AnalyzerType.OAP_ANALYZER,
+ config -> new Gson().fromJson(config.getOapAnalyzer(),
AnalyzerSetting.class)
+ ),
+ OAP_LOG_ANALYZER_SETTING_GENERATOR(
+ Column.AnalyzerType.OAP_LOG_ANALYZER,
+ config -> new Gson().fromJson(config.getOapLogAnalyzer(),
AnalyzerSetting.class)
+ );
+
+ private final Column.AnalyzerType type;
+ private final GenerateAnalyzerSettingFunc func;
+
+ Generator(final Column.AnalyzerType type,
+ final GenerateAnalyzerSettingFunc func) {
+ this.type = type;
+ this.func = func;
+ }
+
+ public GenerateAnalyzerSettingFunc getGenerateFunc() {
+ return this.func;
+ }
+
+ public static Generator GetGenerator(Column.AnalyzerType type) throws
StorageException {
Review comment:
Still have upper case issue.
##########
File path:
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/AnalyzerSetting.java
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.server.storage.plugin.elasticsearch.base;
+
+import com.google.gson.Gson;
+import com.google.gson.annotations.SerializedName;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.storage.StorageException;
+import org.apache.skywalking.oap.server.core.storage.annotation.Column;
+import
org.apache.skywalking.oap.server.storage.plugin.elasticsearch.StorageModuleElasticsearchConfig;
+
+@Getter
+@Setter
+@Slf4j
+public class AnalyzerSetting {
+ /**
+ * A built-in or customised tokenizer.
+ */
+ private Map<String, Object> tokenizer = new HashMap<>();
+ /**
+ * An optional array of built-in or customised character filters.
+ */
+ @SerializedName("char_filter")
+ private Map<String, Object> charFilter = new HashMap<>();
+ /**
+ * An optional array of built-in or customised token filters.
+ */
+ private Map<String, Object> filter = new HashMap<>();
+ /**
+ * The custom analyzers.
+ */
+ private Map<String, Object> analyzer = new HashMap<>();
+
+ public void combine(AnalyzerSetting analyzerSetting) {
+ this.analyzer.putAll(analyzerSetting.getAnalyzer());
+ this.tokenizer.putAll(analyzerSetting.tokenizer);
+ this.filter.putAll(analyzerSetting.filter);
+ this.charFilter.putAll(analyzerSetting.charFilter);
+ }
+
+ @Override
+ public boolean equals(final Object o) {
+ if (this == o)
+ return true;
+ if (!(o instanceof AnalyzerSetting))
+ return false;
+ final AnalyzerSetting that = (AnalyzerSetting) o;
+ return getTokenizer().equals(that.getTokenizer()) &&
+ getCharFilter().equals(that.getCharFilter()) &&
+ getFilter().equals(that.getFilter()) &&
+ getAnalyzer().equals(that.getAnalyzer());
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(getTokenizer(), getCharFilter(), getFilter(),
getAnalyzer());
+ }
+
+ public enum Generator {
+ OAP_ANALYZER_SETTING_GENERATOR(
+ Column.AnalyzerType.OAP_ANALYZER,
+ config -> new Gson().fromJson(config.getOapAnalyzer(),
AnalyzerSetting.class)
+ ),
+ OAP_LOG_ANALYZER_SETTING_GENERATOR(
+ Column.AnalyzerType.OAP_LOG_ANALYZER,
+ config -> new Gson().fromJson(config.getOapLogAnalyzer(),
AnalyzerSetting.class)
+ );
+
+ private final Column.AnalyzerType type;
+ private final GenerateAnalyzerSettingFunc func;
+
+ Generator(final Column.AnalyzerType type,
+ final GenerateAnalyzerSettingFunc func) {
+ this.type = type;
+ this.func = func;
+ }
+
+ public GenerateAnalyzerSettingFunc getGenerateFunc() {
+ return this.func;
+ }
+
+ public static Generator GetGenerator(Column.AnalyzerType type) throws
StorageException {
Review comment:
I think style-check should fail, please check locally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
