fix deleteIndex repeat append namespace by rainbend · Pull Request #3017 · apache/skywalking

rainbend · 2019-07-06T13:09:27Z

Please answer these questions before submitting pull request

Why submit this pull request?
Bug fix
New feature provided
Improve performance
Related issues

Bug fix

Bug description.
org.apache.skywalking.oap.server.storage.plugin.elasticsearch.base.HistoryDeleteEsDAO 的 57行 retrievalIndexByAliases 方法已经把 namespace 加上了 72行的 deleteIndex 方法又加了一遍namespace #3014
How to fix?
#formatIndexName optional

New feature or improvement

Describe the details and related test reports.

wu-sheng

I suggest you move the whole code block inside ElasticSearchClient as method #deleteTimeSeriesData(modelName, timeBefore). Then the delete codes without formatIndexName could ba placed inside method directly.

List<String> indexes = client.retrievalIndexByAliases(model.getName());

            List<String> prepareDeleteIndexes = new ArrayList<>();
            for (String index : indexes) {
                long timeSeries = TimeSeriesUtils.indexTimeSeries(index);
                if (timeBefore >= timeSeries) {
                    prepareDeleteIndexes.add(index);
                }
            }

            if (indexes.size() == prepareDeleteIndexes.size()) {
                client.createIndex(TimeSeriesUtils.timeSeries(model));
            }

            for (String prepareDeleteIndex : prepareDeleteIndexes) {
                client.deleteIndex(prepareDeleteIndex);
            }

I think don't mixe these two kinds of OP(s) together should be much better.

wu-sheng · 2019-07-06T13:58:57Z

.../java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java

+        return deleteIndex(indexName, true);
+    }
+
+    public boolean deleteIndex(String indexName, boolean formatIndexName) throws IOException {


These two delete methods will make people very confused.

I suggest you move the whole code block inside ElasticSearchClient as method #deleteTimeSeriesData(modelName, timeBefore). Then the delete codes without formatIndexName could ba placed inside method directly.

List<String> indexes = client.retrievalIndexByAliases(model.getName()); List<String> prepareDeleteIndexes = new ArrayList<>(); for (String index : indexes) { long timeSeries = TimeSeriesUtils.indexTimeSeries(index); if (timeBefore >= timeSeries) { prepareDeleteIndexes.add(index); } } if (indexes.size() == prepareDeleteIndexes.size()) { client.createIndex(TimeSeriesUtils.timeSeries(model)); } for (String prepareDeleteIndex : prepareDeleteIndexes) { client.deleteIndex(prepareDeleteIndex); }

I think don't mixe these two kinds of OP(s) together should be much better.

This code block needs to use TimeSeriesUtils, but TimeSeriesUtils is located in storage-elasticsearch-plugin

or delete the namespace in the #retrievalIndexByAliasesreturn result.

Then build List ESTimeSeriesIndex, in each element, keep namespace and name in two fields. Then add a new delete method accepting ESTimeSeriesIndex

wu-sheng

Adjust tests, please.

wu-sheng · 2019-07-07T05:38:54Z

.../java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java

+        return indexName;
+    }
+
+    public String undoFormatIndexName(String indexName) {


This should be private.

wu-sheng · 2019-07-07T05:39:23Z

.../java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java

+    }
+
+    public String undoFormatIndexName(String indexName) {
+        if (StringUtils.isNotEmpty(namespace) && indexName.startsWith(namespacePrefix)) {


If !startWith(namespace, then you should raise an exception.

...ava/org/apache/skywalking/oap/server/library/client/elasticsearch/ITElasticSearchClient.java

rainbend · 2019-07-07T08:34:56Z

done

wu-sheng

Questions for test case

wu-sheng · 2019-07-07T15:01:30Z

...ava/org/apache/skywalking/oap/server/library/client/elasticsearch/ITElasticSearchClient.java

-        client = new ElasticSearchClient(esAddress, "", "test", "test");
+        final String esNamespace = System.getProperty("elastic.search.namespace", "");
+        final String esUser = System.getProperty("elastic.search.user", "test");
+        final String esPassword = System.getProperty("elastic.search.password", "test");


One question, who will set these properties?

If not, then, namespace feature hasn't been tested.

Can I use the maven-failsafe-plugin, as follows

<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-failsafe-plugin</artifactId> <configuration> <systemPropertyVariables> <elastic.search.address> ${docker.hostname}:${es-port} </elastic.search.address> <elastic.search.namespace> test </elastic.search.namespace> </systemPropertyVariables> </configuration> <executions> <execution> <goals> <goal>integration-test</goal> <goal>verify</goal> </goals> </execution> </executions> </plugin>

After adding namespace,getIndex has the same problem, which I will modify together

skywalking/oap-server/server-library/library-client/src/main/java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java

Lines 138 to 146 in b21db17

public JsonObject getIndex(String indexName) throws IOException {

indexName = formatIndexName(indexName);

GetIndexRequest request = new GetIndexRequest();

request.indices(indexName);

Response response = client.getLowLevelClient().performRequest(HttpGet.METHOD_NAME, "/" + indexName);

InputStreamReader reader = new InputStreamReader(response.getEntity().getContent());

Gson gson = new Gson();

return gson.fromJson(reader, JsonObject.class);

}

System properties are used for uncertain variables, because of docker env. You don't need this, actually, you could add it to test codes directly, which will be more clear.

Also, @Arugal #getIndex is not used in codes anymore, we only use it in IT test, let's move it to ITElasticSearchClient? That will make us has fewer codes concern of index name.

Also, @Arugal #getIndex is not used in codes anymore, we only use it in IT test, let's move it to ITElasticSearchClient? That will make us has fewer codes concern of index name.

#getIndex needs to invoker RestHighLevelClient#getLowLevelClient ,unless we can get a RestHighLevelClient instance via ElasticSearchClient

System properties are used for uncertain variables, because of docker env. You don't need this, actually, you could add it to test codes directly, which will be more clear.

done

Also, @Arugal #getIndex is not used in codes anymore, we only use it in IT test, let's move it to ITElasticSearchClient? That will make us has fewer codes concern of index name.

#getIndex needs to invoker RestHighLevelClient#getLowLevelClient ,unless we can get a RestHighLevelClient instance via ElasticSearchClient

getIndex method is still there. I think no ine uses it, should remove, right?

Can I do what I said above?

...ava/org/apache/skywalking/oap/server/library/client/elasticsearch/ITElasticSearchClient.java

…pace

wu-sheng

You miss some suggestions from last review.

wu-sheng · 2019-07-08T13:41:18Z

.../java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java

+    private JsonObject undoFormatIndexName(JsonObject index) {
+        if (StringUtils.isNotEmpty(namespace) && index != null && index.size() > 0) {
+            Set<Map.Entry<String, JsonElement>> entrySet = index.entrySet();
+            for (Map.Entry<String, JsonElement> entry : entrySet) {


Change this to index.foreach ->

wu-sheng · 2019-07-08T13:44:03Z

.../java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java

+        return Collections.EMPTY_LIST;
    }

    public JsonObject getIndex(String indexName) throws IOException {


getIndex method needs to move to ITElasticSearchClient with undoFormatIndexName(JsonObject index) method.

wu-sheng · 2019-07-08T13:47:06Z

...che/skywalking/oap/server/library/client/elasticsearch/ITElasticSearchClientOfNamespace.java

+public class ITElasticSearchClientOfNamespace extends ITElasticSearchClient {
+
+
+    public ITElasticSearchClientOfNamespace() {


Has this run? I haven't used this kind of test before. Please confirm in the log.

confirm, This is my local compile log

wu-sheng · 2019-07-08T13:47:53Z

.../java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java

+        return indexName;
+    }
+
+    private JsonObject undoFormatIndexName(JsonObject index) {


We don't need this method, this should move to test class only. Right?

rainbend · 2019-07-09T13:49:35Z

You miss some suggestions from last review.

Do I have any unfinished suggestions now?

wu-sheng · 2019-07-09T14:03:40Z

getIndex method is still there. I think no ine uses it, should remove, right?

peng-yongsheng

I'm not suggest fixed it by this way. You can just add a new delete method without format. It is a very simply way.

wu-sheng · 2019-07-09T14:56:20Z

@peng-yongsheng If you just want an easy way, please consider putting two good method names. Because time series indexes exist, we can't set these methods as #deleteByIndexName and #deleteByModelName.
In time series case, the model A has indexes including A-20190913,A-20190914, etc., but we can't call #deleteByModelName(A)(I know we don't use in this way). But still, show the API is hard named.

peng-yongsheng · 2019-07-09T14:58:18Z

@peng-yongsheng If you just want an easy way, please consider putting two good method names. Because time series indexes exist, we can't set these methods as #deleteByIndexName and #deleteByModelName.
In time series case, the model A has indexes including A-20190913,A-20190914, etc., but we can't call #deleteByModelName(A)(I know we don't use in this way). But still, show the API is hard named.

Agree with my two hands.

rainbend · 2019-07-09T15:01:11Z

@peng-yongsheng If you just want an easy way, please consider putting two good method names. Because time series indexes exist, we can't set these methods as #deleteByIndexName and #deleteByModelName.
In time series case, the model A has indexes including A-20190913,A-20190914, etc., but we can't call #deleteByModelName(A)(I know we don't use in this way). But still, show the API is hard named.

Agree with my two hands.

Now, I only think ElasticSearchTimeSeriesIndex redundant

wu-sheng · 2019-07-09T15:06:12Z

@peng-yongsheng @dmsolr Then this is my suggestion

Change public boolean deleteIndex(String indexName) throws IOException to public boolean deleteByModelName(String noTimeSeriesMode) throws IOException, also add comment to describe the argument.
Add public boolean deleteByIndexName(String indexName) throws IOException, do delete OP w/o format.

Now, I only think ElasticSearchTimeSeriesIndex redundant

Yes. But basically, we are just talking about naming style.

For getIndex, if you need it in ITTest, add a new method in ITTest.

peng-yongsheng · 2019-07-09T15:12:47Z

@peng-yongsheng If you just want an easy way, please consider putting two good method names. Because time series indexes exist, we can't set these methods as #deleteByIndexName and #deleteByModelName.
In time series case, the model A has indexes including A-20190913,A-20190914, etc., but we can't call #deleteByModelName(A)(I know we don't use in this way). But still, show the API is hard named.

Agree with my two hands.

Now, I only think ElasticSearchTimeSeriesIndex redundant

Yes, If you use it, it's not the only place that needs it. It is a strategy that how to manage the namespace. Not just for the delete method. So, I do not suggest you use this way. I had thought it at the time of I program the time series implementation. At that time, I had an ideal same as you. But, If we must keep the overall style of the code. I found there are too many codes that need to modify.

peng-yongsheng · 2019-07-09T15:19:25Z

@peng-yongsheng If you just want an easy way, please consider putting two good method names. Because time series indexes exist, we can't set these methods as #deleteByIndexName and #deleteByModelName.
In time series case, the model A has indexes including A-20190913,A-20190914, etc., but we can't call #deleteByModelName(A)(I know we don't use in this way). But still, show the API is hard named.

Agree with my two hands.

Now, I only think ElasticSearchTimeSeriesIndex redundant

@Arugal
If you interest in how to manage the namespace. I think we can talk about providing a new class named ModelName with at least two attributes. Namespace and model name. And a method to build the final name, and a method to split it. Fixed this PR, think it in the another PR.

rainbend · 2019-07-09T15:28:54Z

If you interest in how to manage the namespace. I think we can talk about providing a new class named ModelName with at least two attributes. Namespace and model name. And a method to build the final name and a method to split it.

Yes, but wouldn't it be better to know the namespace with less code

Fixed this PR, think it in the another PR.

Do I need to make any modification to this PR

peng-yongsheng · 2019-07-09T15:43:55Z

If you interest in how to manage the namespace. I think we can talk about providing a new class named ModelName with at least two attributes. Namespace and model name. And a method to build the final name and a method to split it.

Yes, but wouldn't it be better to know the namespace with less code

No problem, you can new an issue to discuss it.

Fixed this PR, think it in the another PR.

Do I need to make any modification to this PR

Yes, just provide those two methods, '#deleteByIndexName' and '#deleteByModelName'. Rollback other changed files.

wu-sheng · 2019-07-09T23:46:51Z

I still recommend to remove getIndex method, only keep it in test case. Don't revert that.

wu-sheng · 2019-07-10T06:02:34Z

.../java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java

    private final String user;
    private final String password;
-    private RestHighLevelClient client;
+    @Getter(value = AccessLevel.PACKAGE) private RestHighLevelClient client;


Use whitebox get from powermock to get this.

wu-sheng · 2019-07-10T06:31:08Z

/run e2e

wu-sheng

LGTM. @peng-yongsheng please confirm.

peng-yongsheng · 2019-07-10T15:31:26Z

.../java/org/apache/skywalking/oap/server/library/client/elasticsearch/ElasticSearchClient.java

    public ElasticSearchClient(String clusterNodes, String namespace, String user, String password) {
        this.clusterNodes = clusterNodes;
        this.namespace = namespace;
+        this.namespacePrefix = namespace + "_";


I think this.namespacePrefix attribute is no value because we have a common method named formatIndexName.

wu-sheng · 2019-07-10T23:04:07Z

/run ci

wu-sheng · 2019-07-10T23:09:42Z

/run e2e

wu-sheng · 2019-07-11T07:20:30Z

/run ci

wu-sheng · 2019-07-11T13:18:54Z

ASF jenkins break, we are waiting.

wu-sheng · 2019-07-11T22:28:09Z

/run ci

wu-sheng · 2019-07-11T22:28:26Z

/run e2e

fix deleteIndex repeat append namespace

6095b1d

wu-sheng added this to the 6.3.0 milestone Jul 6, 2019

wu-sheng added bug Something isn't working and you are sure it's a bug! backend OAP backend related. labels Jul 6, 2019

wu-sheng requested changes Jul 6, 2019

View reviewed changes

rainbend added 2 commits July 7, 2019 10:37

fix deleteIndex repeat append namespace

4e372c8

fix checkStyle

da3ae0b

wu-sheng reviewed Jul 7, 2019

View reviewed changes

fix

e4ac140

rainbend added 2 commits July 7, 2019 16:38

fix test assert

a0356ee

update test method name

b67ec06

wu-sheng reviewed Jul 7, 2019

View reviewed changes

rainbend and others added 6 commits July 8, 2019 00:15

fix test

f3ebf58

fix

b5c654d

del elastic.search.namespace

c7f48e4

fix check style

b99c495

update ITElasticSearchNamespaceClient to ITElasticSearchClientOfNames…

9ed801d

…pace

Merge branch 'master' into master

67069d8

wu-sheng reviewed Jul 8, 2019

View reviewed changes

rainbend added 2 commits July 8, 2019 22:49

move undoFormatIndexName to ITElasticSearchClien

b532b59

Merge remote-tracking branch 'origin/master'

d6c0175

rainbend closed this Jul 9, 2019

rainbend reopened this Jul 9, 2019

Merge branch 'master' into master

9335f51

rainbend added 2 commits July 9, 2019 22:36

move #getIndex to ITElasticSearchClient

8d14380

Merge remote-tracking branch 'origin/master'

6da11ac

peng-yongsheng requested changes Jul 9, 2019

View reviewed changes

rainbend added 3 commits July 10, 2019 13:45

add deleteByModelName and deleteByIndexName

d2d53b2

add deleteByModelName and deleteByIndexName

9f21430

revert ITElasticSearchClient#getIndex

af792d3

wu-sheng reviewed Jul 10, 2019

View reviewed changes

Use whitebox get from powermock to get this.

f553cd1

wu-sheng previously approved these changes Jul 10, 2019

View reviewed changes

Merge branch 'master' into master

91bef9a

peng-yongsheng requested changes Jul 10, 2019

View reviewed changes

rainbend added 2 commits July 10, 2019 23:45

undo namespacePrefix

d00bd09

Merge remote-tracking branch 'origin/master'

b98424b

rainbend dismissed wu-sheng’s stale review via b98424b July 10, 2019 15:45

peng-yongsheng approved these changes Jul 11, 2019

View reviewed changes

wu-sheng merged commit deb2d90 into apache:master Jul 11, 2019

wu-sheng mentioned this pull request Jul 30, 2019

6.2.0 DataTTLKeeperTimer do not delete the expired records in the table segment #3033

Closed

4 tasks

	public JsonObject getIndex(String indexName) throws IOException {
	indexName = formatIndexName(indexName);
	GetIndexRequest request = new GetIndexRequest();
	request.indices(indexName);
	Response response = client.getLowLevelClient().performRequest(HttpGet.METHOD_NAME, "/" + indexName);
	InputStreamReader reader = new InputStreamReader(response.getEntity().getContent());
	Gson gson = new Gson();
	return gson.fromJson(reader, JsonObject.class);
	}

		public class ITElasticSearchClientOfNamespace extends ITElasticSearchClient {


		public ITElasticSearchClientOfNamespace() {

Conversation

rainbend commented Jul 6, 2019

Bug fix

New feature or improvement

Uh oh!

wu-sheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-sheng Jul 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-sheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rainbend commented Jul 7, 2019

Uh oh!

wu-sheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wu-sheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainbend Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainbend commented Jul 9, 2019

Uh oh!

wu-sheng commented Jul 9, 2019

Uh oh!

peng-yongsheng left a comment

Choose a reason for hiding this comment

wu-sheng Jul 6, 2019 •

edited

Loading

rainbend Jul 8, 2019 •

edited

Loading

rainbend commented Jul 9, 2019 •

edited

Loading

peng-yongsheng commented Jul 9, 2019 •

edited

Loading