aurora-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From wfar...@apache.org
Subject aurora git commit: Fixed issue where saving attributes are not being persisted to log
Date Thu, 02 Nov 2017 21:49:25 GMT
Repository: aurora
Updated Branches:
  refs/heads/master 06d25fd4d -> d10165544


Fixed issue where saving attributes are not being persisted to log

A bug was introduced when the old `MemAttributeStore` was revived. Previously,
the `saveHostAttributes` method did not return anything. However, after
migrating to the DB stores, the signature of the interface was changed to return
a `boolean` if the save modified the previous attributes. The new changes
accidentally inverted the order. The `AbstractAttributeStoreTest` did not test
for this scenario so it went unnoticed.

Reviewed at https://reviews.apache.org/r/63521/


Project: http://git-wip-us.apache.org/repos/asf/aurora/repo
Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/d1016554
Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/d1016554
Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/d1016554

Branch: refs/heads/master
Commit: d1016554408a0b4d6dc6327de60bffb5db260ea5
Parents: 06d25fd
Author: Jordan Ly <jordan.ly8@gmail.com>
Authored: Thu Nov 2 14:49:10 2017 -0700
Committer: Bill Farner <wfarner@apache.org>
Committed: Thu Nov 2 14:49:10 2017 -0700

----------------------------------------------------------------------
 .../storage/mem/MemAttributeStore.java          |  2 +-
 .../storage/AbstractAttributeStoreTest.java     | 40 +++++++++++++-------
 2 files changed, 28 insertions(+), 14 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/aurora/blob/d1016554/src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java
b/src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java
index 483af19..f1a5309 100644
--- a/src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java
+++ b/src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java
@@ -48,7 +48,7 @@ class MemAttributeStore implements AttributeStore.Mutable {
     IHostAttributes previous = hostAttributes.put(
         attributes.getHost(),
         merge(attributes, Optional.fromNullable(hostAttributes.get(attributes.getHost()))));
-    return attributes.equals(previous);
+    return !attributes.equals(previous);
   }
 
   private IHostAttributes merge(IHostAttributes newAttributes, Optional<IHostAttributes>
previous) {

http://git-wip-us.apache.org/repos/asf/aurora/blob/d1016554/src/test/java/org/apache/aurora/scheduler/storage/AbstractAttributeStoreTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/storage/AbstractAttributeStoreTest.java
b/src/test/java/org/apache/aurora/scheduler/storage/AbstractAttributeStoreTest.java
index 34db54b..1ad4680 100644
--- a/src/test/java/org/apache/aurora/scheduler/storage/AbstractAttributeStoreTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/storage/AbstractAttributeStoreTest.java
@@ -33,6 +33,8 @@ import org.junit.Test;
 
 import static org.apache.aurora.gen.MaintenanceMode.DRAINED;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 public abstract class AbstractAttributeStoreTest {
 
@@ -64,27 +66,39 @@ public abstract class AbstractAttributeStoreTest {
   protected abstract Storage createStorage();
 
   @Test
+  public void testSaveAttributes() {
+    assertEquals(Optional.absent(), read(HOST_A));
+
+    // Initial save returns true since it changes previous attributes
+    assertTrue(insert(HOST_A_ATTRS));
+    assertEquals(Optional.of(HOST_A_ATTRS), read(HOST_A));
+
+    // Second save returns false since it does not change previous attributes
+    assertFalse(insert(HOST_A_ATTRS));
+  }
+
+  @Test
   public void testCrud() {
     assertEquals(Optional.absent(), read(HOST_A));
     assertEquals(ImmutableSet.of(), readAll());
 
-    insert(HOST_A_ATTRS);
+    assertTrue(insert(HOST_A_ATTRS));
     assertEquals(Optional.of(HOST_A_ATTRS), read(HOST_A));
     assertEquals(ImmutableSet.of(HOST_A_ATTRS), readAll());
 
-    insert(HOST_B_ATTRS);
-    insert(HOST_B_ATTRS);  // Double insert should be allowed.
+    assertTrue(insert(HOST_B_ATTRS));
+    assertFalse(insert(HOST_B_ATTRS));  // Double insert should be allowed.
     assertEquals(Optional.of(HOST_B_ATTRS), read(HOST_B));
     assertEquals(ImmutableSet.of(HOST_A_ATTRS, HOST_B_ATTRS), readAll());
 
     IHostAttributes updatedA = IHostAttributes.build(
         HOST_A_ATTRS.newBuilder().setAttributes(ImmutableSet.of(ATTR1, ATTR3)));
-    insert(updatedA);
+    assertTrue(insert(updatedA));
     assertEquals(Optional.of(updatedA), read(HOST_A));
     assertEquals(ImmutableSet.of(updatedA, HOST_B_ATTRS), readAll());
 
     IHostAttributes updatedMode = IHostAttributes.build(updatedA.newBuilder().setMode(DRAINED));
-    insert(updatedMode);
+    assertTrue(insert(updatedMode));
     assertEquals(Optional.of(updatedMode), read(HOST_A));
     assertEquals(ImmutableSet.of(updatedMode, HOST_B_ATTRS), readAll());
 
@@ -104,7 +118,7 @@ public abstract class AbstractAttributeStoreTest {
   public void testNoAttributes() {
     IHostAttributes attributes = IHostAttributes.build(
         HOST_A_ATTRS.newBuilder().setAttributes(ImmutableSet.of()));
-    insert(attributes);
+    assertTrue(insert(attributes));
     assertEquals(Optional.of(attributes), read(HOST_A));
   }
 
@@ -121,15 +135,15 @@ public abstract class AbstractAttributeStoreTest {
     HostAttributes attributes = HOST_A_ATTRS.newBuilder();
     attributes.unsetAttributes();
 
-    insert(IHostAttributes.build(attributes));
+    assertTrue(insert(IHostAttributes.build(attributes)));
     assertEquals(Optional.of(IHostAttributes.build(attributes)), read(HOST_A));
   }
 
   @Test
   public void testSlaveIdChanges() {
-    insert(HOST_A_ATTRS);
+    assertTrue(insert(HOST_A_ATTRS));
     IHostAttributes updated = IHostAttributes.build(HOST_A_ATTRS.newBuilder().setSlaveId(SLAVE_B));
-    insert(updated);
+    assertTrue(insert(updated));
     assertEquals(Optional.of(updated), read(HOST_A));
   }
 
@@ -137,7 +151,7 @@ public abstract class AbstractAttributeStoreTest {
   public void testUpdateAttributesWithRelations() {
     // Test for regression of AURORA-1379, where host attribute mutation performed a delete,
     // violating foreign key constraints.
-    insert(HOST_A_ATTRS);
+    assertTrue(insert(HOST_A_ATTRS));
 
     ScheduledTask builder = TaskTestUtil.makeTask("a", JobKeys.from("role", "env", "job"))
         .newBuilder();
@@ -152,12 +166,12 @@ public abstract class AbstractAttributeStoreTest {
     HostAttributes attributeBuilder = HOST_A_ATTRS.newBuilder().setMode(DRAINED);
     attributeBuilder.addToAttributes(new Attribute("newAttr", ImmutableSet.of("a", "b")));
     IHostAttributes hostAUpdated = IHostAttributes.build(attributeBuilder);
-    insert(hostAUpdated);
+    assertTrue(insert(hostAUpdated));
     assertEquals(Optional.of(hostAUpdated), read(HOST_A));
   }
 
-  private void insert(IHostAttributes attributes) {
-    storage.write(
+  private boolean insert(IHostAttributes attributes) {
+    return storage.write(
         storeProvider -> storeProvider.getAttributeStore().saveHostAttributes(attributes));
   }
 


Mime
View raw message