Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provide config stream processor #4133

Merged
merged 5 commits into from
Dec 27, 2019
Merged

provide config stream processor #4133

merged 5 commits into from
Dec 27, 2019

Conversation

mrproliu
Copy link
Contributor

Please answer these questions before submitting pull request

New feature or improvement

  • Provide a new stream processor, because when I create the thread monitor task, need storage similar to inventory and support TTL deletion mode similar to the record.

@wayilau wayilau added the backend OAP backend related. label Dec 26, 2019
@wayilau wayilau added this to the 7.0.0 milestone Dec 26, 2019
@wayilau wayilau added the feature New feature label Dec 26, 2019
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.

Please add comments to all classes. For all new core classes, this should be done.

Most codes are good, just one question about IConfigDAO.

@kezhenxu94 @dmsolr @wayilau @arugal Any interest to follow this PR?

@codecov-io
Copy link

codecov-io commented Dec 26, 2019

Codecov Report

Merging #4133 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4133      +/-   ##
==========================================
- Coverage   27.26%   27.18%   -0.08%     
==========================================
  Files        1141     1145       +4     
  Lines       25057    25100      +43     
  Branches     3625     3629       +4     
==========================================
- Hits         6831     6824       -7     
- Misses      17620    17670      +50     
  Partials      606      606
Impacted Files Coverage Δ
...torage/plugin/elasticsearch/base/StorageEsDAO.java 0% <0%> (ø) ⬆️
...re/analysis/worker/NoneStreamPersistentWorker.java 0% <0%> (ø)
...r/core/analysis/worker/NoneStreamingProcessor.java 0% <0%> (ø)
...server/core/analysis/StreamAnnotationListener.java 0% <0%> (ø) ⬆️
...orage/plugin/elasticsearch7/dao/StorageEs7DAO.java 0% <0%> (ø) ⬆️
...erver/storage/plugin/jdbc/h2/dao/H2StorageDAO.java 0% <0%> (ø) ⬆️
...er/storage/plugin/jdbc/h2/dao/H2NoneStreamDAO.java 0% <0%> (ø)
...age/plugin/elasticsearch/base/NoneStreamEsDAO.java 0% <0%> (ø)
...apm/agent/core/remote/GRPCStreamServiceStatus.java 29.16% <0%> (-25.01%) ⬇️
.../agent/core/context/trace/AbstractTracingSpan.java 60.36% <0%> (-0.91%) ⬇️
... and 4 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 9415f68...cab0750. Read the comment docs.

@kezhenxu94
Copy link
Member

  • Provide a new stream processor, because when I create the thread monitor task, need storage similar to inventory and support TTL deletion mode similar to the record.

@mrproliu Does the configs need TTL and why?

As you wrote in #4144 , monitoring tasks are stored in the inventory here, I suppose the tasks are created by users in the web UI right? So why they should be deleted after a so-called TTL? Does the users expect this? Is it designed for so?

@wu-sheng
Copy link
Member

monitoring tasks are stored in the inventory here, I suppose the tasks are created by users in the web UI right? So why they should be deleted after a so-called TTL? Does the users expect this? Is it designed for so?

Monitoring should be set by UI or GraphQL. But it isn't inventory, that is why we create a new type of streaming process. I think the point of deleting is the thread dump will be saved as a kind of record, similar to the segment. So even we don't set up TTL, it will miss all related thread dump data.
I agree to support TTL.

@mrproliu A reminder to you, as TTL setup based on time bucket, this field should be set based on task expected finish time, rather than config time. Because you need to avoid the task has been deleted before the task starts.

Also, I notice, this task entity is not time series, why? Deleting is a big payload for the ES, I would recommend setup the time series for this too.


Request rename

Config is a very widely used concept. In the streaming process, a mode called config is very strange and not clear. I want to propose the processor name, noneStreamingProcessor.

@arugal
Copy link
Member

arugal commented Dec 27, 2019

So, is Thread Monitor Task Segment snapshot a Config?

@mrproliu
Copy link
Contributor Author

monitoring tasks are stored in the inventory here, I suppose the tasks are created by users in the web UI right? So why they should be deleted after a so-called TTL? Does the users expect this? Is it designed for so?

Monitoring should be set by UI or GraphQL. But it isn't inventory, that is why we create a new type of streaming process. I think the point of deleting is the thread dump will be saved as a kind of record, similar to the segment. So even we don't set up TTL, it will miss all related thread dump data.
I agree to support TTL.

@mrproliu A reminder to you, as TTL setup based on time bucket, this field should be set based on task expected finish time, rather than config time. Because you need to avoid the task has been deleted before the task starts.

Also, I notice, this task entity is not time series, why? Deleting is a big payload for the ES, I would recommend setup the time series for this too.

Request rename

Config is a very widely used concept. In the streaming process, a mode called config is very strange and not clear. I want to propose the processor name, noneStreamingProcessor.

I agree with all your suggestions. I will change that today.

@mrproliu
Copy link
Contributor Author

So, is Thread Monitor Task Segment snapshot a Config?

No, It will create base on Record.

Mrproliu and others added 2 commits December 27, 2019 12:10
2. add comments on none stream relate classes
@mrproliu
Copy link
Contributor Author

@wu-sheng All issue has been resolved.
please have a look.

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.

LGTM. @kezhenxu94 @arugal Recheck?

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

LGTM

@kezhenxu94 kezhenxu94 merged commit 2fcf7c4 into apache:master Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants