[WIP] AFS data store implementation #52
Conversation
Signed-off-by: Giovanni Ferrari <[email protected]>
Signed-off-by: Giovanni Ferrari <[email protected]>
|
@quinarygio : just opening the PR in WIP so that we can comment on it, although it will not pass the CI. |
There was a problem hiding this comment.
@quinarygio :
see some propositions to simplify a little the implementation
|
|
||
| private static final String SEPARATOR = "__"; | ||
|
|
||
| public interface Name { |
There was a problem hiding this comment.
It seems that the implementation is a bit complicated for what we need.
Maybe it was needed for the "data source" in order to handle the suffix and extensions, but I think we should do something simpler here.
I think that here all we need is a naming convention like "DATA_STORE_ENTRY_", + a simple method String getDataName(String entryName).
The day we want to allow a different mapping entry name --> data name, we could pass such a function to the constructor. But for the moment it does not seem useful.
| .build(); | ||
| assertNotNull(importedCase); | ||
| assertEquals("network.tst", importedCase.getName()); | ||
| assertNotNull(importedCase.getNetwork()); |
There was a problem hiding this comment.
I think we should add additional test for the "reading" part, ie the getDataStore method.
We can check that it contains the single entry "network.tst".
| @@ -102,6 +109,11 @@ public ImportedCaseBuilder withDatasource(ReadOnlyDataSource dataSource) { | |||
| return this; | |||
| } | |||
|
|
|||
| public ImportedCaseBuilder withDatastore(ReadOnlyDataStore dataStore) { | |||
There was a problem hiding this comment.
I think we should instead provide directly a DataPack here, because what we need is a consistent set of files.
Possibly we can also have an additional method to take directly a ReadOnlyDataStore, but then it would need a name to search for in the data store.
| // store case data | ||
| importer.copy(dataSource, new AppStorageDataSource(context.getStorage(), info.getId(), info.getName())); | ||
| } else { | ||
| importer = Importers.findImporter(dataStore, name, importersLoader, context.getProject().getFileSystem().getData().getShortTimeExecutionComputationManager(), importConfig); |
There was a problem hiding this comment.
Related to the comment above about using a data pack instead of data store. Here the name is actually the future name of the ImportedCase in AFS, not the name of the network file in the data store, so it will probably not work.
With the data pack, it should be simpler, because you should be able to simply do something like
dataPack.copyTo(new AppStorageDataStore(context.getStorage(), info.getId()));There was a problem hiding this comment.
Ok, I should have read the part below before :)
But the comment is still right: with a data pack as input, you will not even need to check for an importer. Just copy the data and get the format ID from the pack.
| new NodeGenericMetadata().setString(ImportedCase.FORMAT, importer.getFormat())); | ||
| DataFormat df = importer.getDataFormat(); | ||
| try { | ||
| Optional<DataPack> dp = df.newDataResolver().resolve(dataStore, this.name, this.parameters); |
There was a problem hiding this comment.
Should not be needed anymore, but better use optional syntax for the sake of readability:
DataPack dp = df.newDataResolver().resolve(dataStore, this.name, this.parameters)
.orElseThrow(() -> new AfsException("DataPack not resolved"));
dp.copyTo(...);
| @@ -48,9 +51,15 @@ public ReadOnlyDataSource getDataSource() { | |||
| return new AppStorageDataSource(storage, info.getId(), info.getName()); | |||
| } | |||
|
|
|||
| public ReadOnlyDataStore getDataStore() { | |||
There was a problem hiding this comment.
Actually here, I think it would be better to return a DataPack, since an imported case is already supposed to be a consistent network representation (and with an identified format).
If we only return a DataStore, it leaves the user with the work of doing again the resolution work, which he shouldn't need to do.
Signed-off-by: Giovanni Ferrari <[email protected]>
Signed-off-by: Giovanni Ferrari <[email protected]>
Please check if the PR fulfills these requirements (please use
'[x]'to check the checkboxes, or submit the PR and then click the checkboxes)Does this PR already have an issue describing the problem ? If so, link to this issue using
'#XXX'and skip the restWhat kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change or deprecate an API? If yes, check the following:
Other information:
(if any of the questions/checkboxes don't apply, please delete them entirely)