Feature/242 refactor datamanager architecture#256
Merged
abdessamad-abdoun merged 8 commits intoJan 28, 2026
Merged
Conversation
abdessamad-abdoun
added
the
Refactoring
remiceres
reviewed
Jan 26, 2026
| @@ -0,0 +1,64 @@ | |||
| package fr.inria.corese.core.storage.api.dataManager.lifecycle; | |||
Contributor
There was a problem hiding this comment.
Le package fr.inria.corese.core.storage.api.dataManager.lifecycle utilise une majuscule (dataManager). Pour respecter les conventions Java il est préférable d'utiliser uniquement des minuscules
| * @param config New configuration | ||
| * @throws DataManagerException if restart fails | ||
| */ | ||
| default void restart(DataManagerConfig config) throws DataManagerException { |
Contributor
There was a problem hiding this comment.
Atomicité de restart() : La méthode par défaut enchaîne un shutdown() puis un initialize(). Si l'initialisation échoue, le composant reste arrêté et l'état précédent est perdu. Il serait plus robuste d'ajouter un bloc try-catch pour tenter de restaurer l'ancienne configuration en cas d'échec, ou au moins de documenter explicitement ce risque.
| void shutdown() throws DataManagerException; | ||
|
|
||
| /** | ||
| * Restarts the DataManager with a new configuration. |
Contributor
There was a problem hiding this comment.
La Javadoc de restart(config) devrait mentionner qu'elle lance une IllegalArgumentException si la config est null, par souci de cohérence avec initialize(config)
| * | ||
| * @return true if initialized (RUNNING state), false otherwise | ||
| */ | ||
| boolean isInitialized(); |
Contributor
There was a problem hiding this comment.
On a isInitialized() ici et isUsable() dans LifecycleState. Pour éviter les incohérences futures, isInitialized() devrait simplement déléguer à getState().isUsable().
| * | ||
| * @return true if usable (RUNNING) | ||
| */ | ||
| public boolean isUsable() { |
Contributor
There was a problem hiding this comment.
Le nom isUsable() est bien, mais comme l'état s'appelle RUNNING, une méthode isRunning() serait peut-être plus intuitive
| public static final class Builder { | ||
| private final Map<String, Object> properties = new HashMap<>(); | ||
| private boolean transactionSupport = false; | ||
| private String storagePath = "http://ns.inria.fr/corese/dataset"; |
Contributor
There was a problem hiding this comment.
La valeur codée en dur (http://ns.inria.fr/corese/dataset) est très spécifique et arbitraire.
| * @throws DataManagerException if operation fails | ||
| * @throws IllegalArgumentException if context is null | ||
| */ | ||
| MutationResult declareContext(Node context) throws DataManagerException; |
Contributor
There was a problem hiding this comment.
Toutes les méthodes de cette interface concernent des opérations de masse ou structurelles, mais declareContext ne porte que sur un seul Node. Est-ce qu'une version declareContexts(List) ne serait pas plus cohérente avec le nom de l'interface BulkOperations ?
| * @return This builder (for chaining) | ||
| */ | ||
| public Builder storagePath(String path) { | ||
| if (path != null && !path.isEmpty()) { |
Contributor
There was a problem hiding this comment.
Remplacer les vérifications silencieuses (if != null) par des IllegalArgumentException pour les méthodes property() et storagePath().
abdessamad-abdoun
force-pushed
the
feature/242-refactor-datamanager-architecture
branch
from
d58eb66 to
4c63d3d
Compare
|
MaillPierre
approved these changes
Jan 28, 2026
| * @return this builder instance | ||
| * @throws IllegalArgumentException if path is null, empty, or blank | ||
| */ | ||
| public Builder storagePath(String path) { |
Contributor
There was a problem hiding this comment.
I don't get why this is a required parameter ?
What if the datamanager is in-memory ? Is there a storage path ?
| * | ||
| * @return true if predicate is specified | ||
| */ | ||
| public boolean isPredicate() { |
Contributor
There was a problem hiding this comment.
For this function and all the similar one: "isPredicate" -> "hasPredicate"
| */ | ||
| public class GraphStatistics { | ||
|
|
||
| private final long edgeCount; |
Contributor
There was a problem hiding this comment.
Count attributes should not be final, as they will increase or decrease with each modification to the models.
| /** | ||
| * Builder for GraphStatistics. | ||
| */ | ||
| public static class Builder { |
Contributor
There was a problem hiding this comment.
Following comment on R10, not sure if this class is suitable for the builder pattern
abdessamad-abdoun
force-pushed
the
feature/242-refactor-datamanager-architecture
branch
from
6b39e92 to
c1e5458
Compare
|
|
abdessamad-abdoun
deleted the
feature/242-refactor-datamanager-architecture
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.