Skip to content

Conversation

@nisiyong
Copy link
Contributor

@nisiyong nisiyong commented Mar 18, 2021

Update the agent HBase plugin to support HBase Client 2.x

In HBase Client 2.x, the HTable constructor function has a little difference after 2.2.0.
So the agent plugin should have 3 ConstructorInterceptPoint, I have tested it all in test/plugin/scenarios/hbase-scenario, just change the dependency version.

The e2e test cases haven't committed, I will commit it later. Please take a look first, thanks.

@nisiyong nisiyong force-pushed the support-hbase-2.x branch from 41b3a23 to f45ab9c Compare March 18, 2021 11:52
Copy link
Member

Choose a reason for hiding this comment

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

Could you share the naming rule of 100, 200, and 220?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100 -> 1.0.0, means adapt version [1.0.0,1.6.0), 1.6.0 is the latest 1.x currently.
200 -> 2.0.0, means adapt version [2.0.0, 2.1.9), 2.1.9 is the latest 2.1.x currently.
220 -> 2.2.0, means adapt version [2.2.0, 2.4.1), 2.4.1 is the latest 2.x currently.

I use the start version as the name because we don't know what is the newest future version

Copy link
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.

  1. Supported-list.md should be updated
  2. Plugin e2e tests should be updated.

@wu-sheng wu-sheng linked an issue Mar 18, 2021 that may be closed by this pull request
4 tasks
@wu-sheng wu-sheng added agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Mar 18, 2021
Copy link
Member

Choose a reason for hiding this comment

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

Add the comments here about #6577 (comment). We need to be clear about the naming rules.

@codecov
Copy link

codecov bot commented Mar 18, 2021

Codecov Report

Merging #6577 (7f39a00) into master (35d7a52) will increase coverage by 19.75%.
The diff coverage is 0.00%.

❗ Current head 7f39a00 differs from pull request most recent head 79a111a. Consider uploading reports for the commit 79a111a to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             master    #6577       +/-   ##
=============================================
+ Coverage     32.88%   52.64%   +19.75%     
- Complexity     2374     4063     +1689     
=============================================
  Files           996     1796      +800     
  Lines         24360    38521    +14161     
  Branches       2395     4209     +1814     
=============================================
+ Hits           8011    20279    +12268     
- Misses        15755    17261     +1506     
- Partials        594      981      +387     
Impacted Files Coverage Δ Complexity Δ
...walking/apm/plugin/hbase/HTable100Interceptor.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...walking/apm/plugin/hbase/HTable200Interceptor.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...walking/apm/plugin/hbase/HTable220Interceptor.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...skywalking/apm/plugin/hbase/HTableInterceptor.java 0.00% <ø> (ø) 0.00 <0.00> (?)
...andler/v8/grpc/ManagementServiceHandlerCompat.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...erver/receiver/envoy/MetricServiceGRPCHandler.java 31.81% <0.00%> (-61.52%) 3.00% <0.00%> (ø%)
...r/storage/plugin/influxdb/query/EventQueryDAO.java 1.51% <0.00%> (-60.61%) 1.00% <0.00%> (-6.00%)
...er/storage/plugin/jdbc/h2/dao/H2EventQueryDAO.java 1.56% <0.00%> (-46.06%) 1.00% <0.00%> (-3.00%)
...er/receiver/envoy/AccessLogServiceGRPCHandler.java 18.51% <0.00%> (-37.04%) 2.00% <0.00%> (ø%)
...8/grpc/TraceSegmentReportServiceHandlerCompat.java 0.00% <0.00%> (-33.34%) 0.00% <0.00%> (-1.00%)
... and 1402 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 35d7a52...79a111a. Read the comment docs.

@nisiyong nisiyong closed this Mar 19, 2021
@nisiyong nisiyong reopened this Mar 19, 2021
@wu-sheng
Copy link
Member

Please don't close and reopen, You are making all CI process rerun.

@nisiyong
Copy link
Contributor Author

@wu-sheng

After reading these docs as follows:

You said that update the e2e test, does it meaning I just need to update the test/plugin/scenarios/hbase-scenario?
And consider the version, I could remove this module, and add 3 new scenario:

  • hbase-1.x-scenario (just rename hbase-scenario)
  • hbase-2.0.x-scenario
  • hbase-2.2.x-scenario

@wu-sheng
Copy link
Member

You said that update the e2e test, does it meaning I just need to update the test/plugin/scenarios/hbase-scenario?
And consider the version, I could remove this module, and add 3 new scenario:

It depends on which way you prefer? Does the client API change much? Which makes you have to separate the test cases?

@nisiyong
Copy link
Contributor Author

Does the client API change much?

NO, but the HTable constructor function change in different versions.

Which makes you have to separate the test cases?

According to the difference constructor, we need different versions of maven dependency, or we will miss some scenarios.

@wu-sheng
Copy link
Member

NO, but the HTable constructor function change in different versions.

So, you can't use the same test codebase for different versions, right? If so, feel free to separate the cases.

@nisiyong
Copy link
Contributor Author

you can't use the same test codebase for different versions, right?

The test code will be the same or similar, just the maven dependency version is different. We need to cover the HBase agent plugin for the different logic. In my opinion, it still should separate the cases.

@wu-sheng
Copy link
Member

you can't use the same test codebase for different versions, right?

The test code will be the same or similar, just the maven dependency version is different. We need to cover the HBase agent plugin for the different logic. In my opinion, it still should separate the cases.

Then you could add versions into the here, https://github.com/apache/skywalking/blob/master/test/plugin/scenarios/hbase-scenario/support-version.list. The test framework will cover the versions.

@nisiyong nisiyong force-pushed the support-hbase-2.x branch from f45ab9c to f9fbf0e Compare March 24, 2021 12:18
@nisiyong
Copy link
Contributor Author

We need to be clear about the naming rules.

  1. Supported-list.md should be updated
  2. Plugin e2e tests should be updated.

I had added the comment about naming rules and update the support-version.list.
I had already tested the plugin in my local environment, and it works fine.
@wu-sheng PTAL.

@wu-sheng
Copy link
Member

  1. Supported-list.md should be updated

This missed.

@wu-sheng wu-sheng merged commit 51b60ae into apache:master Mar 25, 2021
@nisiyong nisiyong deleted the support-hbase-2.x branch March 25, 2021 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hbase-1.x-plugin is not compatible with 2.x hbase-client

2 participants