You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "mrproliu (via GitHub)" <gi...@apache.org> on 2024/01/07 06:59:38 UTC

[PR] Change the string field in Elasticsearch storage from **keyword** type to *text* if they appoint size more than `32766` [skywalking]

mrproliu opened a new pull request, #11731:
URL: https://github.com/apache/skywalking/pull/11731

   While updating UI templates, I noticed that if the UI template configuration in Elasticsearch exceeds `32766` characters, it fails to save. This is because the keyword type in Elasticsearch supports only up to `32766` characters. The configuration field in the UI template was exceeding this limit. I've changed the **keyword** field to **text** type to successfully update the templates.
   
   - [ ] 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/docs/en/changes/changes.md).
   


-- 
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

Re: [PR] Change the string field in Elasticsearch storage from **keyword** type to *text* if they appoint size more than `32766` [skywalking]

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #11731:
URL: https://github.com/apache/skywalking/pull/11731#discussion_r1443953330


##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -308,7 +308,9 @@ protected Mappings createMapping(Model model) {
         Map<String, Object> properties = new HashMap<>();
         Mappings.Source source = new Mappings.Source();
         for (ModelColumn columnDefine : model.getColumns()) {
-            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(), columnDefine.getElasticSearchExtension());
+            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(),
+                columnDefine.getLength(), columnDefine.isStorageOnly(),
+                columnDefine.getElasticSearchExtension());

Review Comment:
   If only change this method, I think it's not necessary to format other codes.  Hard to find real changes.



-- 
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

Re: [PR] Change the string field in Elasticsearch storage from **keyword** type to *text* if they appoint size more than `32766` [skywalking]

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #11731:
URL: https://github.com/apache/skywalking/pull/11731#discussion_r1443953330


##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -308,7 +308,9 @@ protected Mappings createMapping(Model model) {
         Map<String, Object> properties = new HashMap<>();
         Mappings.Source source = new Mappings.Source();
         for (ModelColumn columnDefine : model.getColumns()) {
-            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(), columnDefine.getElasticSearchExtension());
+            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(),
+                columnDefine.getLength(), columnDefine.isStorageOnly(),
+                columnDefine.getElasticSearchExtension());

Review Comment:
   If only change this method, I think it's not necessary to format other codes. 



-- 
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

Re: [PR] Change the string field in Elasticsearch storage from **keyword** type to *text* if they appoint size more than `32766` [skywalking]

Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #11731:
URL: https://github.com/apache/skywalking/pull/11731#discussion_r1443947707


##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -308,7 +308,8 @@ protected Mappings createMapping(Model model) {
         Map<String, Object> properties = new HashMap<>();
         Mappings.Source source = new Mappings.Source();
         for (ModelColumn columnDefine : model.getColumns()) {
-            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(), columnDefine.getElasticSearchExtension());
+            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(), columnDefine.getLength(),
+                columnDefine.getElasticSearchExtension());

Review Comment:
   updated.



-- 
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

Re: [PR] Change the string field in Elasticsearch storage from **keyword** type to *text* if they appoint size more than `32766` [skywalking]

Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #11731:
URL: https://github.com/apache/skywalking/pull/11731#discussion_r1443955152


##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -308,7 +308,9 @@ protected Mappings createMapping(Model model) {
         Map<String, Object> properties = new HashMap<>();
         Mappings.Source source = new Mappings.Source();
         for (ModelColumn columnDefine : model.getColumns()) {
-            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(), columnDefine.getElasticSearchExtension());
+            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(),
+                columnDefine.getLength(), columnDefine.isStorageOnly(),
+                columnDefine.getElasticSearchExtension());

Review Comment:
   done



-- 
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

Re: [PR] Change the string field in Elasticsearch storage from **keyword** type to *text* if they appoint size more than `32766` [skywalking]

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #11731:
URL: https://github.com/apache/skywalking/pull/11731


-- 
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

Re: [PR] Change the string field in Elasticsearch storage from **keyword** type to *text* if they appoint size more than `32766` [skywalking]

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #11731:
URL: https://github.com/apache/skywalking/pull/11731#discussion_r1443948273


##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/ColumnTypeEsMapping.java:
##########
@@ -28,13 +28,15 @@
 
 public class ColumnTypeEsMapping {
 
-    public String transform(Class<?> type, Type genericType, final ElasticSearchExtension elasticSearchExtension) {
+    public String transform(Class<?> type, Type genericType, int length, boolean storageOnly, final ElasticSearchExtension elasticSearchExtension) {
         if (Integer.class.equals(type) || int.class.equals(type) || Layer.class.equals(type)) {
             return "integer";
         } else if (Long.class.equals(type) || long.class.equals(type)) {
             return "long";
         } else if (Double.class.equals(type) || double.class.equals(type)) {
             return "double";
+        } else if (String.class.equals(type) && storageOnly && length > 32766) {   // 32766 is the max length of a keywords field in ES
+            return "text";

Review Comment:
   @kezhenxu94 @wankai123 This could be a hidden bug, in which length doesn't apply eventually, and fails back to keyword type. The good part is, that this would not break OAP booting.



-- 
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

Re: [PR] Change the string field in Elasticsearch storage from **keyword** type to *text* if they appoint size more than `32766` [skywalking]

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #11731:
URL: https://github.com/apache/skywalking/pull/11731#discussion_r1443947140


##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -308,7 +308,8 @@ protected Mappings createMapping(Model model) {
         Map<String, Object> properties = new HashMap<>();
         Mappings.Source source = new Mappings.Source();
         for (ModelColumn columnDefine : model.getColumns()) {
-            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(), columnDefine.getElasticSearchExtension());
+            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(), columnDefine.getLength(),
+                columnDefine.getElasticSearchExtension());

Review Comment:
   I think we need to check storage only? If it is too long and requires indexing, this convert is not going to work.



-- 
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

Re: [PR] Change the string field in Elasticsearch storage from **keyword** type to *text* if they appoint size more than `32766` [skywalking]

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #11731:
URL: https://github.com/apache/skywalking/pull/11731#discussion_r1443954033


##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/base/StorageEsInstaller.java:
##########
@@ -308,7 +308,9 @@ protected Mappings createMapping(Model model) {
         Map<String, Object> properties = new HashMap<>();
         Mappings.Source source = new Mappings.Source();
         for (ModelColumn columnDefine : model.getColumns()) {
-            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(), columnDefine.getElasticSearchExtension());
+            final String type = columnTypeEsMapping.transform(columnDefine.getType(), columnDefine.getGenericType(),
+                columnDefine.getLength(), columnDefine.isStorageOnly(),
+                columnDefine.getElasticSearchExtension());

Review Comment:
   @mrproliu Could you try to revert other parts of the codes?



-- 
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