[bisq-network/bisq] Persistence redesign (#4589)

[sqrrm]

@sqrrm requested changes on this pull request.



> +        openInstances.decrementAndGet();
+        if (openInstances.get() == 0) {

To reduce confusion, how about
```suggestion
        if (openInstances.decrementAndGet() == 0) {
```

> +    ///////////////////////////////////////////////////////////////////////////////////////////
+
+    @Inject
+    public PersistenceManager(@Named(Config.STORAGE_DIR) File dir,
+                              PersistenceProtoResolver persistenceProtoResolver,
+                              CorruptedStorageFileHandler corruptedStorageFileHandler) {
+        this.dir = checkDir(dir);
+        this.persistenceProtoResolver = persistenceProtoResolver;
+        this.corruptedStorageFileHandler = corruptedStorageFileHandler;
+    }
+
+    ///////////////////////////////////////////////////////////////////////////////////////////
+    // API
+    ///////////////////////////////////////////////////////////////////////////////////////////
+
+    public void initialize(T persistable, Source sourcce) {

Misspelled `sourcce`

> +        }
+        return null;
+    }
+
+
+    ///////////////////////////////////////////////////////////////////////////////////////////
+    // Write file to disk
+    ///////////////////////////////////////////////////////////////////////////////////////////
+
+    public void requestPersistence() {
+        persistenceRequested = true;
+
+        // We write to disk with a delay to avoid frequent write operations. Depending on the priority those delays
+        // can be rather long.
+        if (timer == null) {
+            timer = UserThread.runPeriodically(() -> {

Why use `runPeriodically()` instead of `runAfter()`? The effect would be the same but now it seems the write is intended to happen periodically, but two lines later that we see that it's not.

>      }
 
     public PersistableList(List<T> list) {
-        this.list = list;
+        setAll(list);
+    }
+
+    public void addListener(ListChangeListener<T> listener) {
+        ((ObservableList<T>) getList()).addListener(listener);

This does not look safe. I think `list` should be an `ObservableList` since it's required by this interface. It's first initiated to an `ArrayList` (line 39) but I assume all implementations of `PersistableList` passes an `ObservableList` as the constructor argument.

> +    }
+
+    public void setAll(Collection<T> collection) {
+        this.list.clear();
+        this.list.addAll(collection);
+    }
+
+    public boolean add(T item) {
+        if (!list.contains(item)) {
+            list.add(item);
+            return true;
+        }
+        return false;
+    }
+
+    public boolean remove(T tradable) {

Naming the generic `tradable` is confusing, `item` is more neutral

>      public Stream<T> stream() {
         return list.stream();
     }
 
-    private interface ExcludesDelegateMethods<T> {
-        Stream<T> stream();
+    public int size() {
+        return list.size();
+    }
+
+    public boolean contains(T thing) {

Rather use a uniform naming for the generic, like `item`

> @@ -148,21 +153,30 @@ protected Injector getInjector() {
     }
 
     protected void applyInjector() {
-        setupPersistedDataHosts(injector);
+        // Subclassed might configure classes with the injector here

```suggestion
        // Subclasses might configure classes with the injector here
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/4589#pullrequestreview-501550596
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20201004/31a4ee2e/attachment.html>