-
Notifications
You must be signed in to change notification settings - Fork 24
Remove customized data providers from manager #179
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
Remove customized data providers from manager #179
Conversation
1701206 to
0512250
Compare
...ipse/tracecompass/incubator/internal/inandout/core/analysis/InAndOutDataProviderFactory.java
Outdated
Show resolved
Hide resolved
0512250 to
028b651
Compare
| InAndOutAnalysisModule.remove(config, trace); | ||
| } | ||
|
|
||
| private static void removeChildrenDataProviders(ITmfTrace trace, ITmfConfiguration config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I like this implementation a lot better. But I can't help but think that this would need to be done by all factories that support configurations. It could have been the new API in DataProviderManager: removeProviders(ITmfTrace, ITmfConfiguration), that would remove all data providers that have this configuration, instead of per data provider id.
I also wonder if, in this implementation, the first call to the method would also cover the removal of the data provider that is being explicitly removed by the DataProviderService in the previous commit.
So I'm thinking, in DataProviderService.deleteDerivedProvider(), could it just call something like DataProviderManager.removeProviders(ITmfTrace, ITmfConfiguration), and then the configurators/factories only need to worry about removing the descriptor and configuration? They are not really the owners of the data provider instances.
One consideration is that either DataProviderService or DataProviderManager would need to do this processing for the full trace set (experiment and its child traces).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting comment. It's the same idea than for the first commit, but it would take care of all sub data providers. I'll try it. It requires another change in mainline TC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created eclipse-tracecompass/org.eclipse.tracecompass#257 and apply in this PR.
...ipse/tracecompass/incubator/internal/inandout/core/analysis/InAndOutDataProviderFactory.java
Outdated
Show resolved
Hide resolved
028b651 to
a726160
Compare
...cecompass/incubator/internal/trace/server/jersey/rest/core/services/DataProviderService.java
Show resolved
Hide resolved
fixes eclipse-tracecompass-incubator#178 Signed-off-by: Bernd Hufmann <[email protected]>
a726160 to
152cad2
Compare
48b9008
into
eclipse-tracecompass-incubator:master
What it does
Remove any instances of derived data providers and sub-data providers created with InAndAnalysis.
fixes #178
Requires the following Trace Compass PRs:
How to test
Note: The same issue exists for the 3 other derived views and it is fixed with this PR.
Follow-ups
Review checklist
Signed-off-by: Bernd Hufmann [email protected]