You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sentry.apache.org by va...@apache.org on 2017/06/14 05:58:30 UTC

sentry git commit: SENTRY-1796: Add better debug logging for retrieving the delta changes (Vamsee Yarlagadda, Reviewed by: Alex Kolbasov)

Repository: sentry
Updated Branches:
  refs/heads/sentry-ha-redesign e44fa87e9 -> acf5debe3


SENTRY-1796: Add better debug logging for retrieving the delta changes
(Vamsee Yarlagadda, Reviewed by: Alex Kolbasov)


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

Branch: refs/heads/sentry-ha-redesign
Commit: acf5debe3c6dffd2c519f2394b1c27d1b1429379
Parents: e44fa87
Author: Vamsee Yarlagadda <va...@cloudera.com>
Authored: Wed Jun 7 18:00:49 2017 -0700
Committer: Vamsee Yarlagadda <va...@cloudera.com>
Committed: Tue Jun 13 22:57:12 2017 -0700

----------------------------------------------------------------------
 .../sentry/core/common/utils/SentryUtils.java   |  77 +++++++++++++
 .../core/common/utils/TestSentryUtils.java      |  60 +++++++++++
 .../db/service/model/MSentryChange.java         |   1 +
 .../provider/db/service/model/MSentryUtil.java  |  58 +++++++++-
 .../db/service/persistent/SentryStore.java      |  96 +++++++++++------
 .../db/service/model/TestMSentryUtil.java       | 107 +++++++++++++++++++
 6 files changed, 364 insertions(+), 35 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/sentry/blob/acf5debe/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
----------------------------------------------------------------------
diff --git a/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java b/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
new file mode 100644
index 0000000..b225af0
--- /dev/null
+++ b/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.sentry.core.common.utils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Classes to hold general utility functions that could be used across Sentry
+ */
+public final class SentryUtils {
+
+  /**
+   * Given a list of sorted numbers, return the string that prints in the collapsed format.
+   * <p>
+   * e.g:
+   * <li> Input: [1, 2, 3, 5, 7] </li>
+   * <li> Output: "[1-3, 5, 7]" </li>
+   * </p>
+   * @param nums List of sorted numbers
+   * @return Collapsed string representation of the numbers
+   */
+  public static String collapseNumsToString(List<Long> nums) {
+    List<String> collapsedStrings = new ArrayList<>();
+
+    // Using startIndex, movingIndex we maintain a variable size moving window across the Array where at any
+    // point in time, startIndex points to the beginning of a continuous sequence, movingIndex points to the last known
+    // sequential number of that particular subsequence and currentGroupSize captures the size of the subsequence.
+    int startIndex = 0;
+    int movingIndex = 0;
+    int currentGroupSize = 0;
+    int listSize = nums.size();
+
+    // Using the startIndex as terminating condition to capture the last sequence(before the list terminates)
+    // within the loop itself to avoid code duplication.
+    while (startIndex < listSize) {
+      if (movingIndex == listSize ||
+          nums.get(startIndex) + currentGroupSize != nums.get(movingIndex)) {
+        long startID = nums.get(startIndex);
+        long endID = nums.get(movingIndex - 1);
+        if (startID == endID) {
+          collapsedStrings.add(String.format("%d", startID));
+        } else if (endID - startID == 1) {
+          collapsedStrings.add(String.format("%d", startID));
+          collapsedStrings.add(String.format("%d", endID));
+        } else {
+          collapsedStrings.add(String.format("%d-%d", startID, endID));
+        }
+
+        startIndex = movingIndex;
+        currentGroupSize = 0;
+        continue;
+      }
+
+      movingIndex++;
+      currentGroupSize++;
+    }
+
+    return collapsedStrings.toString();
+  }
+}

http://git-wip-us.apache.org/repos/asf/sentry/blob/acf5debe/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestSentryUtils.java
----------------------------------------------------------------------
diff --git a/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestSentryUtils.java b/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestSentryUtils.java
new file mode 100644
index 0000000..0c04df8
--- /dev/null
+++ b/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestSentryUtils.java
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.sentry.core.common.utils;
+
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.Collections;
+
+import static org.junit.Assert.assertEquals;
+
+public class TestSentryUtils {
+
+  @Test
+  public void testCollapseNumsToString() throws Exception {
+    assertEquals("Collapsed string should match",
+                 SentryUtils.collapseNumsToString(Arrays.asList(1L)),
+                 "[1]");
+
+    assertEquals("Collapsed string should match",
+                 SentryUtils.collapseNumsToString(Arrays.asList(1L, 2L)),
+                 "[1, 2]");
+
+    assertEquals("Collapsed string should match",
+                 SentryUtils.collapseNumsToString(Arrays.asList(1L, 2L, 4L)),
+                 "[1, 2, 4]");
+
+    assertEquals("Collapsed string should match",
+                 SentryUtils.collapseNumsToString(Arrays.asList(1L, 2L, 4L, 5L)),
+                 "[1, 2, 4, 5]");
+
+    assertEquals("Collapsed string should match",
+                 SentryUtils.collapseNumsToString(Arrays.asList(1L, 2L, 4L, 5L, 6L)),
+                 "[1, 2, 4-6]");
+
+    assertEquals("Collapsed string should match",
+                 SentryUtils.collapseNumsToString(Arrays.asList(1L, 2L, 4L, 5L, 6L, 8L)),
+                 "[1, 2, 4-6, 8]");
+
+    assertEquals("Collapsed string should just return empty string",
+                 SentryUtils.collapseNumsToString(Collections.<Long>emptyList()),
+                 "[]");
+  }
+}

http://git-wip-us.apache.org/repos/asf/sentry/blob/acf5debe/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java
index 6011ef4..9b022a1 100644
--- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java
+++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java
@@ -21,4 +21,5 @@ package org.apache.sentry.provider.db.service.model;
  * The base class for various delta changes stored in Sentry DB.
  */
 public interface MSentryChange {
+  long getChangeID();
 }

http://git-wip-us.apache.org/repos/asf/sentry/blob/acf5debe/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
index 7558267..939bf83 100644
--- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
+++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
@@ -18,10 +18,17 @@
 
 package org.apache.sentry.provider.db.service.model;
 
+import org.apache.sentry.core.common.utils.SentryUtils;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+
 /**
  * Common utilities for model objects
  */
-final class MSentryUtil {
+public final class MSentryUtil {
   /**
    * Intern a string but only if it is not null
    * @param arg String to be interned, may be null
@@ -30,4 +37,53 @@ final class MSentryUtil {
   static String safeIntern(String arg) {
     return (arg != null) ? arg.intern() : null;
   }
+
+  /**
+   * Given a collection of MSentryChange's, retrieve the change id's associated and return as a list
+   * <p>
+   * e.g:
+   * <li> Input: [MSentryChange(1), MSentryChange(2), MSentryChange(3), MSentryChange(5), MSentryChange(7)] </li>
+   * <li> Output: [1, 2, 3, 5 ,7] </li>
+   * </p>
+   * @param changes List of {@link MSentryChange}
+   * @return List of changeID's
+   */
+  private static List<Long> getChangeIds(Collection<? extends MSentryChange> changes) {
+    List<Long> ids = changes.isEmpty() ? Collections.<Long>emptyList() : new ArrayList<Long>(changes.size());
+    for (MSentryChange change : changes) {
+      ids.add(change.getChangeID());
+    }
+    return ids;
+  }
+
+  /**
+   * Given a collection of MSentryChange instances sorted by ID return true if and only if IDs are sequential (do not contain holes)
+   * <p>
+   * e.g:
+   * <li> Input: [MSentryChange(1), MSentryChange(2), MSentryChange(3), MSentryChange(5), MSentryChange(7)] </li>
+   * <li> Output: False </li>
+   * <li> Input: [MSentryChange(1), MSentryChange(2), MSentryChange(3), MSentryChange(4), MSentryChange(5)] </li>
+   * <li> Output: True </li>
+   * </p>
+   * @param changes List of {@link MSentryChange}
+   * @return True if all the ids are sequential otherwise returns False
+   */
+  public static boolean isConsecutive(List<? extends MSentryChange> changes) {
+    int size = changes.size();
+    return (size <= 1) || (changes.get(size - 1).getChangeID() - changes.get(0).getChangeID() + 1 == size);
+  }
+
+  /**
+   * Given a collection of MSentryChange instances sorted by ID, return the string that prints in the collapsed format.
+   * <p>
+   * e.g:
+   * <li> Input: [MSentryChange(1), MSentryChange(2), MSentryChange(3), MSentryChange(5), MSentryChange(7)] </li>
+   * <li> Output: "[1-3, 5, 7]" </li>
+   * </p>
+   * @param changes  List of {@link MSentryChange}
+   * @return Collapsed string representation of the changeIDs
+   */
+  public static String collapseChangeIDsToString(Collection<? extends MSentryChange> changes) {
+    return SentryUtils.collapseNumsToString(getChangeIds(changes));
+  }
 }

http://git-wip-us.apache.org/repos/asf/sentry/blob/acf5debe/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
index 17856a4..187b38b 100644
--- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
+++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
@@ -63,6 +63,7 @@ import org.apache.sentry.provider.db.service.model.MSentryPrivilege;
 import org.apache.sentry.provider.db.service.model.MSentryUser;
 import org.apache.sentry.provider.db.service.model.MSentryVersion;
 import org.apache.sentry.provider.db.service.model.MSentryRole;
+import org.apache.sentry.provider.db.service.model.MSentryUtil;
 import org.apache.sentry.provider.db.service.model.MPath;
 import org.apache.sentry.provider.db.service.thrift.SentryPolicyStoreProcessor;
 import org.apache.sentry.provider.db.service.thrift.TSentryActiveRoleSet;
@@ -3743,70 +3744,97 @@ public class SentryStore {
 
   /**
    * Gets a list of MSentryPathChange objects greater than or equal to the given changeID.
-   * If there is any path deltas missing in {@link MSentryPathChange} table, which means
-   * the size of retrieved paths deltas is less than the requested one, an empty list will
-   * be returned to caller.
+   * If there is any path delta missing in {@link MSentryPathChange} table, an empty list is returned.
    *
-   * @param changeID
-   * @return a list of MSentryPathChange objects. It can returns an empty list.
+   * @param changeID  Requested changeID
+   * @return a list of MSentryPathChange objects. May be empty.
    * @throws Exception
    */
   public List<MSentryPathChange> getMSentryPathChanges(final long changeID)
-      throws Exception {
+          throws Exception {
     return tm.executeTransaction(new TransactionBlock<List<MSentryPathChange>>() {
       public List<MSentryPathChange> execute(PersistenceManager pm) throws Exception {
+        // 1. We first retrieve the entire list of latest delta changes since the changeID
         List<MSentryPathChange> pathChanges =
-            getMSentryChangesCore(pm, MSentryPathChange.class, changeID);
-        long curChangeID = getLastProcessedChangeIDCore(pm, MSentryPathChange.class);
-        long expectedSize = curChangeID - changeID + 1;
-        long actualSize = pathChanges.size();
-        if (actualSize < expectedSize) {
-          LOGGER.error(String.format("Certain path delta is missing in " +
-              "SENTRY_PATH_CHANEG table! Current size of elements = %s and expected size = %s, " +
-              "from changeID: %s. The table may get corrupted.",
-              actualSize, expectedSize, changeID));
-          return Collections.emptyList();
-        } else {
+                getMSentryChangesCore(pm, MSentryPathChange.class, changeID);
+        // 2. We then check for consistency issues with delta changes
+        if (validateDeltaChanges(changeID, pathChanges)) {
+          // If everything is in order, return the delta changes
           return pathChanges;
         }
+
+        // Looks like delta change validation failed. Return an empty list.
+        return Collections.emptyList();
       }
     });
   }
 
   /**
    * Gets a list of MSentryPermChange objects greater than or equal to the given ChangeID.
-   * If there is any perm deltas missing in {@link MSentryPermChange} table, which means
-   * the size of retrieved perm deltas is less than the requested one, an empty list will
-   * be returned to caller.
+   * If there is any path delta missing in {@link MSentryPermChange} table, an empty list is returned.
    *
-   * @param changeID
-   * @return a list of MSentryPermChange objects
+   * @param changeID Requested changeID
+   * @return a list of MSentryPathChange objects. May be empty.
    * @throws Exception
    */
   public List<MSentryPermChange> getMSentryPermChanges(final long changeID)
       throws Exception {
-    return tm.executeTransaction(
-    new TransactionBlock<List<MSentryPermChange>>() {
+    return tm.executeTransaction(new TransactionBlock<List<MSentryPermChange>>() {
       public List<MSentryPermChange> execute(PersistenceManager pm) throws Exception {
+        // 1. We first retrieve the entire list of latest delta changes since the changeID
         List<MSentryPermChange> permChanges =
             getMSentryChangesCore(pm, MSentryPermChange.class, changeID);
-        long curChangeID = getLastProcessedChangeIDCore(pm, MSentryPermChange.class);
-        long expectedSize = curChangeID - changeID + 1;
-        long actualSize = permChanges.size();
-        if (actualSize < expectedSize) {
-          LOGGER.error(String.format("Certain perm delta is missing in " +
-             "SENTRY_PERM_CHANEG table! Current size of elements = %s and expected size = %s, " +
-              "from changeID: %s. The table may get corrupted.",
-              actualSize, expectedSize, changeID));
-          return Collections.emptyList();
-        } else {
+        // 2. We then check for consistency issues with delta changes
+        if (validateDeltaChanges(changeID, permChanges)) {
+          // If everything is in order, return the delta changes
           return permChanges;
         }
+
+        // Looks like delta change validation failed. Return an empty list.
+        return Collections.emptyList();
       }
     });
   }
 
   /**
+   * Validate if the delta changes are consistent with the requested changeID.
+   * <p>
+   *   1. Checks if the first delta changeID matches the requested changeID
+   *   (This verifies the delta table still has entries starting from the changeID) <br/>
+   *   2. Checks if there are any holes in the list of delta changes <br/>
+   * </p>
+   * @param changeID Requested changeID
+   * @param changes List of delta changes
+   * @return True if the delta changes are all consistent otherwise returns False
+   */
+  public <T extends MSentryChange> boolean validateDeltaChanges(final long changeID, List<T> changes) {
+    if (changes.isEmpty()) {
+      // If database has nothing more recent than CHANGE_ID return True
+      return true;
+    }
+
+    // The first delta change from the DB should match the changeID
+    // If it doesn't, then it means the delta table no longer has entries starting from the requested CHANGE_ID
+    if (changes.get(0).getChangeID() != changeID) {
+      LOGGER.debug(String.format("Starting delta change from %s is off from the requested id. " +
+          "Requested changeID: %s, Missing delta count: %s",
+          changes.get(0).getClass().getCanonicalName(), changeID, changes.get(0).getChangeID() - changeID));
+      return false;
+    }
+
+    // Check for holes in the delta changes.
+    if (!MSentryUtil.isConsecutive(changes)) {
+      String pathChangesIds = MSentryUtil.collapseChangeIDsToString(changes);
+      LOGGER.error(String.format("Certain delta is missing in %s! The table may get corrupted. " +
+          "Start changeID %s, Current size of elements = %s. path changeID list: %s",
+          changes.get(0).getClass().getCanonicalName(), changeID, changes.size(), pathChangesIds));
+      return false;
+    }
+
+    return true;
+  }
+
+  /**
    * Execute Perm/Path UpdateTransaction and corresponding actual
    * action transaction, e.g dropSentryRole, in a single transaction.
    * Note that this method only applies to TransactionBlock that

http://git-wip-us.apache.org/repos/asf/sentry/blob/acf5debe/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java
----------------------------------------------------------------------
diff --git a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java
new file mode 100644
index 0000000..eb39b0d
--- /dev/null
+++ b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java
@@ -0,0 +1,107 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.sentry.provider.db.service.model;
+
+import org.apache.sentry.hdfs.PathsUpdate;
+import org.apache.sentry.hdfs.PermissionsUpdate;
+
+import org.apache.sentry.hdfs.Updateable;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class TestMSentryUtil {
+
+  @Test
+  public void testMSentryUtilWithPathChanges() throws Exception {
+    List<MSentryPathChange> changes = new ArrayList<>();
+    PathsUpdate update = new PathsUpdate(1, false);
+
+    changes.add(new MSentryPathChange(1, update));
+    assertEquals("Collapsed string should match", "[1]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertTrue("List of changes should be consecutive", MSentryUtil.isConsecutive(changes));
+
+    changes.add(new MSentryPathChange(2, update));
+    assertEquals("Collapsed string should match", "[1, 2]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertTrue("List of changes should be consecutive", MSentryUtil.isConsecutive(changes));
+
+    changes.add(new MSentryPathChange(4, update));
+    assertEquals("Collapsed string should match", "[1, 2, 4]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertFalse("List of changes should not be consecutive", MSentryUtil.isConsecutive(changes));
+
+    changes.add(new MSentryPathChange(5, update));
+    assertEquals("Collapsed string should match", "[1, 2, 4, 5]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertFalse("List of changes should not be consecutive", MSentryUtil.isConsecutive(changes));
+
+    changes.add(new MSentryPathChange(6, update));
+    assertEquals("Collapsed string should match", "[1, 2, 4-6]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertFalse("List of changes should not be consecutive", MSentryUtil.isConsecutive(changes));
+
+    changes.add(new MSentryPathChange(8, update));
+    assertEquals("Collapsed string should match", "[1, 2, 4-6, 8]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertFalse("List of changes should not be consecutive", MSentryUtil.isConsecutive(changes));
+  }
+
+  @Test
+  public void testMSentryUtilWithPermChanges() throws Exception {
+    List<MSentryPermChange> changes = new ArrayList<>();
+    PermissionsUpdate update = new PermissionsUpdate(1, false);
+
+    changes.add(new MSentryPermChange(1, update));
+    assertEquals("Collapsed string should match", "[1]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertTrue("List of changes should be consecutive", MSentryUtil.isConsecutive(changes));
+
+    changes.add(new MSentryPermChange(2, update));
+    assertEquals("Collapsed string should match", "[1, 2]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertTrue("List of changes should be consecutive", MSentryUtil.isConsecutive(changes));
+
+    changes.add(new MSentryPermChange(4, update));
+    assertEquals("Collapsed string should match", "[1, 2, 4]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertFalse("List of changes should not be consecutive", MSentryUtil.isConsecutive(changes));
+
+    changes.add(new MSentryPermChange(5, update));
+    assertEquals("Collapsed string should match", "[1, 2, 4, 5]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertFalse("List of changes should not be consecutive", MSentryUtil.isConsecutive(changes));
+
+    changes.add(new MSentryPermChange(6, update));
+    assertEquals("Collapsed string should match", "[1, 2, 4-6]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertFalse("List of changes should not be consecutive", MSentryUtil.isConsecutive(changes));
+
+    changes.add(new MSentryPermChange(8, update));
+    assertEquals("Collapsed string should match", "[1, 2, 4-6, 8]",
+        MSentryUtil.collapseChangeIDsToString(changes));
+    assertFalse("List of changes should not be consecutive", MSentryUtil.isConsecutive(changes));
+  }
+}