Skip to content

Upgrade the InfluxDB storage-plugin to protocol V3#4641

Merged
wu-sheng merged 29 commits intoapache:masterfrom
dmsolr:v8/influxdb
Apr 19, 2020
Merged

Upgrade the InfluxDB storage-plugin to protocol V3#4641
wu-sheng merged 29 commits intoapache:masterfrom
dmsolr:v8/influxdb

Conversation

@dmsolr
Copy link
Copy Markdown
Member

@dmsolr dmsolr commented Apr 12, 2020

Please answer these questions before submitting pull request

@dmsolr dmsolr added the backend OAP backend related. label Apr 12, 2020
@dmsolr dmsolr added this to the 8.0.0 milestone Apr 12, 2020
@dmsolr dmsolr requested review from kezhenxu94 and wu-sheng April 12, 2020 18:10
Copy link
Copy Markdown
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to discuss about isTag and tagName in the Column annotation.

  1. When do we need isTag?
  2. Once the isTag == true, tagName could be a auto generated information, such as in the H2/MySQL storage, index name is auto generation, is there any reason, you can't do this?

The key point is, I want to make the storage more general. Like your comments of this field, Whether the column is stored as Tag on time-series DB., this is very specific, but other all existing annotation attributes are general, only search may be a little closer to the ElasticSearch as it has a better search feature.

Let's discuss this deeper, to find out whether we could reduce them or replace them with more clear name.
Because at the entity annotation level, it should not link to the storage directly so much. Otherwise, anyone wants to add a entity, need to understand all storage implementation, it will increase the bar for the contributors.

@wu-sheng
Copy link
Copy Markdown
Member

@kezhenxu94 WDYT about this direction?

Copy link
Copy Markdown
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the limited tags today. Once we have time on the performance test, we will have more ideas about the improvements.

Let's add the e2e back.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 14, 2020

Codecov Report

Merging #4641 into master will increase coverage by 0.27%.
The diff coverage is 49.47%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4641      +/-   ##
============================================
+ Coverage     49.12%   49.39%   +0.27%     
- Complexity     2512     2562      +50     
============================================
  Files          1267     1266       -1     
  Lines         27577    27699     +122     
  Branches       3001     3032      +31     
============================================
+ Hits          13547    13683     +136     
+ Misses        13386    13374      -12     
+ Partials        644      642       -2     
Impacted Files Coverage Δ Complexity Δ
...torage/plugin/influxdb/query/AggregationQuery.java 12.50% <0.00%> (+12.50%) 2.00 <0.00> (+2.00)
...server/storage/plugin/influxdb/query/LogQuery.java 6.34% <0.00%> (+6.34%) 2.00 <0.00> (+2.00)
...er/storage/plugin/influxdb/query/MetricsQuery.java 2.61% <0.00%> (+2.61%) 2.00 <0.00> (+2.00)
...torage/plugin/influxdb/query/TopNRecordsQuery.java 11.76% <0.00%> (+11.76%) 2.00 <0.00> (+2.00)
...r/storage/plugin/influxdb/query/TopologyQuery.java 3.50% <0.00%> (+3.50%) 2.00 <0.00> (+2.00)
...rver/storage/plugin/influxdb/query/TraceQuery.java 40.36% <25.00%> (+40.36%) 6.00 <0.00> (+6.00)
...gin/influxdb/query/ProfileThreadSnapshotQuery.java 87.50% <38.46%> (+87.50%) 11.00 <1.00> (+11.00)
.../plugin/influxdb/query/NetworkAddressAliasDAO.java 41.37% <41.37%> (ø) 3.00 <3.00> (?)
...p/server/storage/plugin/influxdb/InfluxClient.java 55.76% <50.00%> (+55.76%) 12.00 <1.00> (+12.00)
...r/storage/plugin/influxdb/query/MetadataQuery.java 52.70% <52.70%> (ø) 11.00 <11.00> (?)
... and 80 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21bb638...365b7bb. Read the comment docs.

Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rewrite the docker-compose.yml to extend from the base-compose.yml, these codes should be counted in the code coverage, please refer to other docker-compose.yml or ping me if you have any problem

@dmsolr
Copy link
Copy Markdown
Member Author

dmsolr commented Apr 17, 2020

Now, it is ready for review. @mrproliu @wu-sheng @kezhenxu94

@dmsolr dmsolr changed the title [WIP]Upgrade the InfluxDB storage-plugin to protocol V3 Upgrade the InfluxDB storage-plugin to protocol V3 Apr 17, 2020
kezhenxu94
kezhenxu94 previously approved these changes Apr 18, 2020
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just review the E2E part, look good to me

wu-sheng
wu-sheng previously approved these changes Apr 18, 2020
Copy link
Copy Markdown
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If coverage is on, good to me. I am doing storage interface refactor, So, this would be changed again.

@dmsolr dmsolr dismissed stale reviews from wu-sheng and kezhenxu94 via 316fcbc April 18, 2020 10:36
wu-sheng
wu-sheng previously approved these changes Apr 18, 2020
@dmsolr
Copy link
Copy Markdown
Member Author

dmsolr commented Apr 19, 2020

@kezhenxu94 , could you help me to see es6 always failed on e2e? I have not modified this partial. (It is also failed on #4666)

wu-sheng
wu-sheng previously approved these changes Apr 19, 2020
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The InfluxDB storage test failed, and I believe it's related to the code itself, @dmsolr please confirm through the logs, there're exceptions related to the codes

@wu-sheng
Copy link
Copy Markdown
Member

@kezhenxu94 It seems good now, Need your approve

@wu-sheng wu-sheng merged commit 28530cd into apache:master Apr 19, 2020
ascrutae pushed a commit to ascrutae/sky-walking that referenced this pull request Apr 19, 2020
Co-authored-by: 吴晟 Wu Sheng <wu.sheng@foxmail.com>
Co-authored-by: kezhenxu94 <kezhenxu94@163.com>
Co-authored-by: kezhenxu94 <kezhenxu94@apache.org>
@dmsolr dmsolr mentioned this pull request Apr 20, 2020
4 tasks
@kezhenxu94 kezhenxu94 linked an issue Apr 21, 2020 that may be closed by this pull request
wu-sheng pushed a commit that referenced this pull request Apr 22, 2020
1. Re-balance the workload of plugintests
2. Restore testcases of MySQL, remove on #4641
3. Support user to runs plugin-test on debug mode. It will not remove the log files and actualData.yaml
4. remove paramater 'parallel_run_size' and disable parallel test.
@dmsolr dmsolr deleted the v8/influxdb branch April 26, 2020 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend OAP backend related.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add influx db as an available storage option again.

5 participants