jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] Load existing machines info on startup (#355)
Date Tue, 31 Jan 2017 21:41:46 GMT
nacx commented on this pull request.

Looks good in general! I've added a few comments to complete my understanding on how the provider
works..
Could you also elaborate a bit the motivation behind having split the Vagrant Facade in two
different interfaces?

>        this.timeSupplier = timeSupplier;
+      this.nodes = Suppliers.memoize(new ConcurrentWrapperSupplier(existingMachines));

Would it make sense to configure this supplier (qualified with `@Memoized` if appropriate)
in a Guice module? It looks more flexible and correct to get injected what you're gonna use
instead of decorating the arguments?

> @@ -73,36 +93,41 @@ public long getDelay(TimeUnit unit) {
          return unit.convert(expiryTime - timeSupplier.get(), TimeUnit.MILLISECONDS);
       }
    }
-   private Map<String, VagrantNode> nodes = new ConcurrentHashMap<String, VagrantNode>();
-   private DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
+
+   private final DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();

Didn't know about this type of queue. The way it is used here looks awesome! :)

> @@ -73,36 +93,41 @@ public long getDelay(TimeUnit unit) {
          return unit.convert(expiryTime - timeSupplier.get(), TimeUnit.MILLISECONDS);
       }
    }
-   private Map<String, VagrantNode> nodes = new ConcurrentHashMap<String, VagrantNode>();
-   private DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();
+
+   private final DelayQueue<TerminatedNode> terminatedNodes = new DelayQueue<TerminatedNode>();

Just to understand the whole picture. Is this used as an in-memory cache of recently terminated
nodes (and why)? If not, is there any other purpose for this?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/355#pullrequestreview-19429117
Mime
View raw message