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/03/15 06:11:40 UTC
[GitHub] [skywalking] wankai123 opened a new pull request #8679: Add storage column attribute `indexOnly`
wankai123 opened a new pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679
### Add storage column attribute `indexOnly`
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [X] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [X] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/CHANGES.md).
Add storage column attribute `indexOnly`, support ElasticSearch only index, and not store some fields.
Add `indexOnly=true` to `SegmentRecord.tags`, to reduce unnecessary storage.
--
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
[GitHub] [skywalking] wu-sheng commented on a change in pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#discussion_r827702108
##########
File path: CHANGES.md
##########
@@ -109,6 +109,9 @@ Release Notes.
* [Breaking Change] Remove configuration `restAcceptorPriorityDelta` (env var: `SW_RECEIVER_SHARING_JETTY_DELTA`
, `SW_CORE_REST_JETTY_DELTA`).
* [Breaking Change] Remove configuration `graphql/path` (env var: `SW_QUERY_GRAPHQL_PATH`).
+* [Breaking Change] Remove configuration `restMinThreads` (env var: `SW_CORE_REST_JETTY_MIN_THREADS`, `SW_RECEIVER_SHARING_JETTY_MIN_THREADS`).
Review comment:
Is this missed from the previous PR? @kezhenxu94
--
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
[GitHub] [skywalking] wankai123 commented on a change in pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
wankai123 commented on a change in pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#discussion_r827668419
##########
File path: oap-server/server-library/library-elasticsearch-client/src/test/java/org/apache/skywalking/library/elasticsearch/ITElasticSearchTest.java
##########
@@ -209,6 +226,15 @@ public void testSearch() {
assertEquals(1, response.getHits().getTotal());
assertEquals("val1", response.getHits().iterator().next().getSource().get("key1"));
});
+ //test indexOnly
+ await().atMost(Duration.ONE_MINUTE).untilAsserted(() -> {
+ SearchResponse response = client.search(
+ Search.builder().query(Query.bool().must(Query.term("key3", "val3"))).build()
+ );
+ assertEquals(1, response.getHits().getTotal());
+ assertEquals("val1", response.getHits().iterator().next().getSource().get("key1"));
+ assertNull(response.getHits().iterator().next().getSource().get("key3"));
+ });
Review comment:
Good suggestion!
--
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
[GitHub] [skywalking] wankai123 commented on pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
wankai123 commented on pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#issuecomment-1067736181
hold, more codes need to adjust.
--
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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#discussion_r826618294
##########
File path: oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/response/Mappings.java
##########
@@ -42,4 +44,9 @@
@Getter
@Setter
private Map<String, Object> properties = new HashMap<>();
+
+ @JsonProperty("_source")
+ @Getter
+ @Setter
+ private Map<String, Object> source = new HashMap<>();
Review comment:
This is unused?
--
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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#discussion_r827707887
##########
File path: CHANGES.md
##########
@@ -109,6 +109,9 @@ Release Notes.
* [Breaking Change] Remove configuration `restAcceptorPriorityDelta` (env var: `SW_RECEIVER_SHARING_JETTY_DELTA`
, `SW_CORE_REST_JETTY_DELTA`).
* [Breaking Change] Remove configuration `graphql/path` (env var: `SW_QUERY_GRAPHQL_PATH`).
+* [Breaking Change] Remove configuration `restMinThreads` (env var: `SW_CORE_REST_JETTY_MIN_THREADS`, `SW_RECEIVER_SHARING_JETTY_MIN_THREADS`).
Review comment:
This was duplicate maybe because of resolving conflict
--
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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#discussion_r826618959
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/manual/segment/SegmentRecord.java
##########
@@ -91,7 +90,7 @@
private byte[] dataBinary;
@Setter
@Getter
- @Column(columnName = TAGS)
+ @Column(columnName = TAGS, indexOnly = true)
Review comment:
If the tags are only indexed, how can they be displayed in the UI, or we don't display these ever?
--
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
[GitHub] [skywalking] wu-sheng commented on a change in pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#discussion_r827708398
##########
File path: CHANGES.md
##########
@@ -109,6 +109,9 @@ Release Notes.
* [Breaking Change] Remove configuration `restAcceptorPriorityDelta` (env var: `SW_RECEIVER_SHARING_JETTY_DELTA`
, `SW_CORE_REST_JETTY_DELTA`).
* [Breaking Change] Remove configuration `graphql/path` (env var: `SW_QUERY_GRAPHQL_PATH`).
+* [Breaking Change] Remove configuration `restMinThreads` (env var: `SW_CORE_REST_JETTY_MIN_THREADS`, `SW_RECEIVER_SHARING_JETTY_MIN_THREADS`).
Review comment:
```suggestion
```
--
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
[GitHub] [skywalking] wu-sheng commented on a change in pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#discussion_r827705461
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/storage/annotation/Column.java
##########
@@ -58,6 +58,12 @@
*/
boolean storageOnly() default false;
+ /**
+ * The column(field) is just indexed, never stored. Note: this feature only supported by elasticsearch
+ * and can not support update mappings in current index
Review comment:
```suggestion
* and don't support mappings update due to ElasticSearch server's limitation.
*
* NOTICE, metrics should not use this, as the OAP core merges indices of metrics automatically.
```
`can't` usually means we can not originally.
--
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
[GitHub] [skywalking] wankai123 commented on a change in pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
wankai123 commented on a change in pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#discussion_r826613579
##########
File path: oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java
##########
@@ -148,7 +149,7 @@ private void createTimeSeriesTable(Model model) throws StorageException {
if (shouldUpdateTemplate) {
structures.putStructure(tableName, mapping);
boolean isAcknowledged = esClient.createOrUpdateTemplate(
- tableName, settings, structures.getMapping(tableName), config.getIndexTemplateOrder());
+ tableName, settings, mapping, config.getIndexTemplateOrder());
Review comment:
I'm not sure why use `structures.getMapping(tableName)` before. In that logic, the updated template would only add new mapping property but wouldn't update/delete the existing property.
--
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
[GitHub] [skywalking] wankai123 commented on a change in pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
wankai123 commented on a change in pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#discussion_r826624430
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/manual/segment/SegmentRecord.java
##########
@@ -91,7 +90,7 @@
private byte[] dataBinary;
@Setter
@Getter
- @Column(columnName = TAGS)
+ @Column(columnName = TAGS, indexOnly = true)
Review comment:
All segment info include `tags` are stored in `dataBinary` already.
--
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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#discussion_r827655343
##########
File path: oap-server/server-library/library-elasticsearch-client/src/test/java/org/apache/skywalking/library/elasticsearch/ITElasticSearchTest.java
##########
@@ -177,13 +189,18 @@ public void testDoc() {
@Test
public void testSearch() {
final String index = "test-index";
+ final Mappings.SourceConf sourceConf = new Mappings.SourceConf();
+ sourceConf.getExcludes().add("key3");
assertTrue(
client.index().create(
index,
Mappings.builder()
.type("type")
.properties(ImmutableMap.of("key1", ImmutableMap.of("type", "keyword")))
- .properties(ImmutableMap.of("key2", ImmutableMap.of("type", "keyword")))
+ .properties(ImmutableMap.of("key2", ImmutableMap.of("type", "keyword"), "key3",
+ ImmutableMap.of("type", "keyword")
Review comment:
```suggestion
.properties(ImmutableMap.of("key2", ImmutableMap.of("type", "keyword"),
"key3", ImmutableMap.of("type", "keyword")
```
##########
File path: oap-server/server-library/library-elasticsearch-client/src/test/java/org/apache/skywalking/library/elasticsearch/ITElasticSearchTest.java
##########
@@ -209,6 +226,15 @@ public void testSearch() {
assertEquals(1, response.getHits().getTotal());
assertEquals("val1", response.getHits().iterator().next().getSource().get("key1"));
});
+ //test indexOnly
+ await().atMost(Duration.ONE_MINUTE).untilAsserted(() -> {
+ SearchResponse response = client.search(
+ Search.builder().query(Query.bool().must(Query.term("key3", "val3"))).build()
+ );
+ assertEquals(1, response.getHits().getTotal());
+ assertEquals("val1", response.getHits().iterator().next().getSource().get("key1"));
+ assertNull(response.getHits().iterator().next().getSource().get("key3"));
+ });
Review comment:
Prefer an error message than comments so that when it failed we can see the reason from the logs directly
```suggestion
await().atMost(Duration.ONE_MINUTE).untilAsserted(() -> {
SearchResponse response = client.search(
Search.builder().query(Query.bool().must(Query.term("key3", "val3"))).build()
);
assertEquals(1, response.getHits().getTotal());
assertEquals("val1", response.getHits().iterator().next().getSource().get("key1"));
assertNull("indexOnly fields should not be stored",
response.getHits().iterator().next().getSource().get("key3"));
});
```
##########
File path: oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/response/Mappings.java
##########
@@ -42,4 +45,16 @@
@Getter
@Setter
private Map<String, Object> properties = new HashMap<>();
+
+ @JsonProperty("_source")
+ @Getter
+ @Setter
+ private SourceConf source = new SourceConf();
+
+ public static class SourceConf {
Review comment:
Let's simplify the name
```suggestion
public static class Source {
```
--
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
[GitHub] [skywalking] wankai123 commented on pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
wankai123 commented on pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679#issuecomment-1068760705
@kezhenxu94 @wu-sheng could review now
--
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
[GitHub] [skywalking] wu-sheng merged pull request #8679: Add storage column attribute `indexOnly`
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #8679:
URL: https://github.com/apache/skywalking/pull/8679
--
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