Propose new api for configurable data provider factories #235
Conversation
|
NOTES:
|
| description = "Data provider defined by configuration " + configuration.getName(); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| TmfConfiguration.Builder builder = new TmfConfiguration.Builder(); |
There was a problem hiding this comment.
This class currently enforces the use of TmfConfiguration, A better approach might be to introduce a createConfig() method, which child classes can implement to return an ITmfConfiguration.
@Override
protected ITmfConfiguration createConfig() {
return new CustomTmfConfiguration();
}
| List<ITmfConfiguration> configs = readConfigurations(tr); | ||
| readAndApplyConfiguration(trace, configs); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
The configuration might be stored at the experiment level, so it might be better to move the following two lines outside the else block to ensure they always execute.
List<ITmfConfiguration> configs = readConfigurations(trace);
readAndApplyConfiguration(trace, configs);
| * The json file extension | ||
| * @since 9.5 | ||
| */ | ||
| public static final String JSON_EXTENSION = "json"; //$NON-NLS-1$ |
There was a problem hiding this comment.
We can't remove it just yet. It's api and needs to be deprecated first. In incubator master branch there is already a new class that uses it.
| * if an error occurs | ||
| * @since 9.5 | ||
| */ | ||
| public static void writeConfiguration(ITmfConfiguration configuration, IPath rootPath) throws TmfConfigurationException { |
There was a problem hiding this comment.
We can't remove it just yet. It's api and needs to be deprecated first. In incubator master branch there is already a new class that uses it.
| /** | ||
| * This class meant to be extended by data provider factories that want to be | ||
| * able to handle configurations. | ||
| */ |
There was a problem hiding this comment.
you need a @since 9.6 tag
| /** | ||
| * @return a table mapping configuration id and trace (exp) to its configuration | ||
| */ | ||
| protected Table<String, ITmfTrace, ITmfConfiguration> getConfigurationTable(){ |
There was a problem hiding this comment.
If you put this public, then you can have the configurator as a separate class and the parent data provider factory doesn't need to extend that class (see IDataProviderFactory.getAdapter(), which needs to be overriden by the factory to return other objects that itself). I tried it with the InAndOutAnalysis and it worked.
| @@ -0,0 +1,242 @@ | |||
| package org.eclipse.tracecompass.tmf.core.config; | |||
| * the close signal to handle | ||
| */ | ||
| @TmfSignalHandler | ||
| public void traceClosed(TmfTraceClosedSignal signal) { |
There was a problem hiding this comment.
This this class handles tmf signals, we need to register to the signal manager and also derigster when it's disposed. You can register in the constructor.
TmfSignalManager.register(this);Probably this class should have a dispose method to deregister from the signal manager. The factory dispose() method would call then this method.
What it does
Proposal to extend the public API with a new class that can (should) be extended by all data provider factories that want to provide the configuration feature.
The main goal is to provide a "pattern" for configurable data provider factories, so that they follow a similar coding style (at least for the less advanced use cases). This hopefully can help to make configurable data provider factories easier to implement and to document how to implement them.
A practical example of how to use this is provided in a separate PR for the InAndOutDataProvider in the incubator project (I will add comments below).
How to test
Tests will be added if the proposal is accepted.
For the moment, checking out this branch together with the one named similarly for the incubator project, where the InAndOutDataProvider is used, it should enable seeing how the code changes while the InAndOutDataProvider has the same functions as before.
Follow-ups
If accepted, adding tests and documentation on how to create configurable data provider factories using this common pattern.
Review checklist