Merge lp:~makyo/juju-gui/endpoints-once into lp:juju-gui/experimental

[Madison Scott-Clary]

Clean up get_endpoints

get_endpoints was being called on every instance of service_add and not being garbage collected. Moved it to on_database_changed and added gc around services being removed.

https://codereview.appspot.com/6858099/

[Madison Scott-Clary (makyo)]

Reviewers: mp+137273_code.launchpad.net,

Message:
Please take a look.

Description:
Clean up get_endpoints

get_endpoints was being called on every instance of service_add and not
being garbage collected. Moved it to on_database_changed and added gc
around services being removed.

https://code.launchpad.net/~makyo/juju-gui/endpoints-once/+merge/137273

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6858099/

Affected files:
   A [revision details]
   M app/app.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: app/app.js
=== modified file 'app/app.js'
--- app/app.js 2012-11-20 16:55:43 +0000
+++ app/app.js 2012-11-30 17:33:27 +0000
@@ -256,8 +256,6 @@
        // TODO - Bound views will automatically update this on individual
models
        this.db.on('update', this.on_database_changed, this);

- this.db.services.on('add', this.updateEndpoints, this);
-
        this.on('navigate', function(e) {
          console.log('app navigate', e);
        });
@@ -310,6 +308,35 @@
       */
      on_database_changed: function(evt) {
        Y.log(evt, 'debug', 'App: Database changed');
+
+ var updateNeeded = false,
+ self = this;
+ if (!Y.Lang.isValue(this.serviceEndpoints)) {
+ this.serviceEndpoints = {};
+ }
+
+ // Compare endpoints map against db to see if it needs to be changed.
+ this.db.services.some(function(service) {
+ if (self.serviceEndpoints[service.get('id')] === undefined) {
+ updateNeeded = true;
+ }
+ return updateNeeded;
+ });
+
+ // If there are new services in the DB, pull an updated endpoints
map.
+ if (updateNeeded) {
+ this.updateEndpoints();
+ } else {
+ // Check to see if any services have been removed (if there are,
and
+ // new ones also, updateEndpoints will replace the whole map, so
only
+ // do this if needed).
+ Y.Object.each(this.serviceEndpoints, function(key, value, obj) {
+ if (self.db.services.getById(key) === null) {
+ delete(self.serviceEndpoints[key]);
+ }
+ });
+ }
+
        // Redispatch to current view to update.
        this.dispatch();
      },
@@ -322,9 +349,6 @@
      updateEndpoints: function(callback) {
        var self = this;

- if (!Y.Lang.isValue(this.serviceEndpoints)) {
- this.serviceEndpoints = {};
- }
        // Defensive code to aid tests. Other systems
        // don't have to mock enough to get_endpoints below.
        if (!this.env.get('connected')) {

[Kapil Thangavelu (hazmat)]

Missing a unit test.

Else looks good.

Merge with changes.

Thanks

https://codereview.appspot.com/6858099/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6858099/diff/1/app/app.js#newcode314
app/app.js:314: if (!Y.Lang.isValue(this.serviceEndpoints)) {
i'd suggest moving this to the initializer and dropping the conditional
instead of lazy value initialization.

https://codereview.appspot.com/6858099/diff/1/app/app.js#newcode323
app/app.js:323: return updateNeeded;
return is extraneous here

https://codereview.appspot.com/6858099/

[lp:~makyo/juju-gui/endpoints-once]

263. By Madison Scott-Clary on 2012-12-03

Clean up usage of get_endpoints in app so that it's only called when needed.

264. By Madison Scott-Clary on 2012-12-03

Clean up usage of get_endpoints in app so that it's only called when needed.

[Madison Scott-Clary (makyo)]

Please take a look.

https://codereview.appspot.com/6858099/

[Brad Crittenden (bac)]

Looks good but I've suggested a code simplification. No need to
re-review.

https://codereview.appspot.com/6858099/diff/5001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6858099/diff/5001/app/app.js#newcode325
app/app.js:325:
.some() returns true or false so I think you could just do:

var updateNeeded = this.db.services.some(function(service) {
          return (self.serviceEndpoints[service.get('id')] ===
undefined)});

https://codereview.appspot.com/6858099/

[Madison Scott-Clary (makyo)]

*** Submitted:

Clean up get_endpoints

get_endpoints was being called on every instance of service_add and not
being garbage collected. Moved it to on_database_changed and added gc
around services being removed.

R=
CC=
https://codereview.appspot.com/6858099

https://codereview.appspot.com/6858099/

=== modified file 'app/app.js'
--- app/app.js	2012-11-20 16:55:43 +0000
+++ app/app.js	2012-12-03 20:28:23 +0000
@@ -256,8 +256,6 @@
       // TODO - Bound views will automatically update this on individual models
       this.db.on('update', this.on_database_changed, this);
 
-      this.db.services.on('add', this.updateEndpoints, this);
-
       this.on('navigate', function(e) {
         console.log('app navigate', e);
       });
@@ -310,6 +308,35 @@
      */
     on_database_changed: function(evt) {
       Y.log(evt, 'debug', 'App: Database changed');
+
+      var updateNeeded = false,
+          self = this;
+      if (!Y.Lang.isValue(this.serviceEndpoints)) {
+        this.serviceEndpoints = {};
+      }
+
+      // Compare endpoints map against db to see if it needs to be changed.
+      this.db.services.some(function(service) {
+        if (self.serviceEndpoints[service.get('id')] === undefined) {
+          updateNeeded = true;
+        }
+        return updateNeeded;
+      });
+
+      // If there are new services in the DB, pull an updated endpoints map.
+      if (updateNeeded) {
+        this.updateEndpoints();
+      } else {
+        // Check to see if any services have been removed (if there are, and
+        // new ones also, updateEndpoints will replace the whole map, so only
+        // do this if needed).
+        Y.Object.each(this.serviceEndpoints, function(key, value, obj) {
+          if (self.db.services.getById(key) === null) {
+            delete(self.serviceEndpoints[key]);
+          }
+        });
+      }
+
       // Redispatch to current view to update.
       this.dispatch();
     },
@@ -322,9 +349,6 @@
     updateEndpoints: function(callback) {
       var self = this;
 
-      if (!Y.Lang.isValue(this.serviceEndpoints)) {
-        this.serviceEndpoints = {};
-      }
       // Defensive code to aid tests. Other systems
       // don't have to mock enough to get_endpoints below.
       if (!this.env.get('connected')) {
 
=== modified file 'test/test_app.js'
--- test/test_app.js	2012-11-27 14:25:32 +0000
+++ test/test_app.js	2012-12-03 20:28:23 +0000
@@ -10,7 +10,7 @@
       ['relation', 'add',
        {'interface': 'reversenginx', 'scope': 'global',
          'endpoints': [['wordpress', {'role': 'peer', 'name': 'loadbalancer'}]],
-         'id': 'relation-0000000000'}],
+         'id': 'relation-0000000002'}],
       ['relation', 'add',
        {'interface': 'mysql',
          'scope': 'global', 'endpoints':
@@ -186,7 +186,7 @@
             env: env,
             charm_store: {} });
 
-      app.updateEndpoints = function() {};
+      //app.updateEndpoints = function() {};
       env.get_endpoints = function() {};
     });
 
@@ -208,5 +208,33 @@
       // Tests of the actual load machinery are in the model and env tests, and
       // so are not repeated here.
     });
+
+    it('must request endpoints only when necessary', function() {
+      var get_endpoints_count = 0,
+          tmp_data = {
+            result: [
+              ['service', 'add', {
+                'charm': 'cs:precise/mysql-6',
+                'id': 'mysql2'
+              }],
+              ['unit', 'add', {
+                'machine': 0,
+                'agent-state': 'started',
+                'public-address': '192.168.122.222',
+                'id': 'mysql2/0'
+              }]
+            ],
+            op: 'delta'
+          };
+      env.get_endpoints = function(services, callback) {
+        get_endpoints_count += 1;
+      };
+      // Inject default data, should only get_endpoints once.
+      injectData(app);
+      get_endpoints_count.should.equal(1);
+      // Additional deltas should only call get_endpoints once.
+      app.db.on_delta({ data: tmp_data });
+      get_endpoints_count.should.equal(2);
+    });
   });
 });