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 <[email protected]> Authored: Wed Jun 7 18:00:49 2017 -0700 Committer: Vamsee Yarlagadda <[email protected]> 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)); + } +}
