You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "nscendoni (via GitHub)" <gi...@apache.org> on 2023/09/21 10:41:27 UTC
[GitHub] [jackrabbit-oak] nscendoni opened a new pull request, #1128: OAK-10451 UserPrincipalProvider may cause many conflicts when under load
nscendoni opened a new pull request, #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1371688768
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) {
}
//------------------------------------------------------------< private >---
+
+ GroupPrincipalFactory createGroupPrincipalFactory() {
Review Comment:
return value should be annotated with @NotNull
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) {
}
//------------------------------------------------------------< private >---
+
+ GroupPrincipalFactory createGroupPrincipalFactory() {
Review Comment:
return value should be annotated with @NotNull
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1348475708
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+public class UserPrincipalProviderCommitterProvider {
+
+ static UserPrincipalProviderCommitterProvider instance = null;
+ static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>();
Review Comment:
I modified it in a way that the provider has a method to remove elements from the map, and the thread call it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1371652407
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupMembershipReader.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.jetbrains.annotations.NotNull;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+
+/**
+ * <code>GroupMembershipReader</code>...
+ */
+class GroupMembershipReader {
+
+ private final MembershipProvider membershipProvider;
+
+ protected final GroupPrincipalFactory groupPrincipalFactory;
+
+ GroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory) {
+ this.membershipProvider = checkNotNull(membershipProvider);
+ this.groupPrincipalFactory = checkNotNull(groupPrincipalFactory);
+ }
+
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+
+ protected final void loadGroupPrincipals(@NotNull Tree authorizableTree,
Review Comment:
same here. why does this need to be 'protected'?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) {
}
//------------------------------------------------------------< private >---
+
+ GroupPrincipalFactory createGroupPrincipalFactory() {
Review Comment:
the return value should be annotated with '@NotNull' and the methods should be private.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupMembershipReader.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.jetbrains.annotations.NotNull;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+
+/**
+ * <code>GroupMembershipReader</code>...
+ */
+class GroupMembershipReader {
+
+ private final MembershipProvider membershipProvider;
+
+ protected final GroupPrincipalFactory groupPrincipalFactory;
+
+ GroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory) {
+ this.membershipProvider = checkNotNull(membershipProvider);
+ this.groupPrincipalFactory = checkNotNull(groupPrincipalFactory);
+ }
+
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+
+ protected final void loadGroupPrincipals(@NotNull Tree authorizableTree,
+ @NotNull Set<Principal> groupPrincipals) {
+ Iterator<Tree> groupTrees = membershipProvider.getMembership(authorizableTree, true);
+ while (groupTrees.hasNext()) {
+ Tree groupTree = groupTrees.next();
+ if (UserUtil.isType(groupTree, AuthorizableType.GROUP)) {
+ Principal gr = groupPrincipalFactory.create(groupTree);
+ if (gr != null) {
+ groupPrincipals.add(gr);
+ }
+ }
+ }
+ }
+
+ interface GroupPrincipalFactory {
+
+ Principal create(@NotNull Tree authorizable);
Review Comment:
return value should be annotated with NotNull or Nullable
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
+ */
+class CachedGroupMembershipReader extends GroupMembershipReader {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class);
+
+ private static final long MEMBERSHIP_THRESHOLD = 0;
+
+ /**
+ * Keep track of cache updates for 100 most recent authorizables.
+ */
+ private static final int MAX_CACHE_TRACKING_ENTRIES = 100;
+
+ private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>();
+
+ private final UserConfiguration config;
+
+ private final Root root;
+
+ private final long expiration;
+ private final long maxStale;
+
+ CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory,
+ @NotNull UserConfiguration config,
+ @NotNull Root root) {
+ super(membershipProvider, groupPrincipalFactory);
+ this.config = checkNotNull(config);
+ this.root = root;
+ this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE);
+ this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE);
+ }
+
+ @Override
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ // membership cache only implemented on user
+ if (UserUtil.isType(authorizable, AuthorizableType.USER)) {
+ readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals);
+ } else {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+ }
+
+ private void readGroupsFromCache(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groups,
+ @NotNull BiConsumer<Tree, Set<Principal>> loader) {
+ long expirationTime = 0;
+ String authorizablePath = authorizableNode.getPath();
+ Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (principalCache.exists()) {
+ expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE);
+ }
+ long now = System.currentTimeMillis();
+ if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) {
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ // need to load or serve stale if allowed
+ boolean mayServeStale = now - expirationTime < maxStale;
+ boolean updateCache;
+ Map<String, Long> updates = getCacheUpdateMap();
+ synchronized (updates) {
+ // anyone else updating that entry already?
Review Comment:
instead of asking a question, i would find the comment a lot more helpful if it would explain. "test if the cache for the given user is already being updated. this is achieved by ....." (please fill in .... to make it easily understandable for anyone looking at this code. at a first glance that was immediately obvious to me and how the exp time play a role here)
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) {
}
//------------------------------------------------------------< private >---
+
+ GroupPrincipalFactory createGroupPrincipalFactory() {
Review Comment:
return value should be annotated with @NotNull
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
+ */
+class CachedGroupMembershipReader extends GroupMembershipReader {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class);
+
+ private static final long MEMBERSHIP_THRESHOLD = 0;
+
+ /**
+ * Keep track of cache updates for 100 most recent authorizables.
+ */
+ private static final int MAX_CACHE_TRACKING_ENTRIES = 100;
+
+ private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>();
+
+ private final UserConfiguration config;
+
+ private final Root root;
+
+ private final long expiration;
+ private final long maxStale;
+
+ CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory,
+ @NotNull UserConfiguration config,
+ @NotNull Root root) {
+ super(membershipProvider, groupPrincipalFactory);
+ this.config = checkNotNull(config);
+ this.root = root;
+ this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE);
+ this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE);
+ }
+
+ @Override
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ // membership cache only implemented on user
+ if (UserUtil.isType(authorizable, AuthorizableType.USER)) {
+ readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals);
+ } else {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+ }
+
+ private void readGroupsFromCache(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groups,
+ @NotNull BiConsumer<Tree, Set<Principal>> loader) {
+ long expirationTime = 0;
+ String authorizablePath = authorizableNode.getPath();
+ Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (principalCache.exists()) {
+ expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE);
+ }
+ long now = System.currentTimeMillis();
+ if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) {
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ // need to load or serve stale if allowed
+ boolean mayServeStale = now - expirationTime < maxStale;
+ boolean updateCache;
+ Map<String, Long> updates = getCacheUpdateMap();
+ synchronized (updates) {
+ // anyone else updating that entry already?
+ Long exp = updates.get(authorizablePath);
+ updateCache = exp == null || expirationTime > exp;
+ if (updateCache) {
+ // we do it
+ updates.put(authorizablePath, expirationTime);
+ }
+ }
+
+ if (!updateCache && mayServeStale) {
+ LOG.debug("Another thread is updating the cache and we may serve stale");
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ if (updateCache) {
+ boolean cacheSuccess = false;
+ try {
+ loader.accept(authorizableNode, groups);
+ cacheGroups(authorizableNode, groups);
+ cacheSuccess = true;
+ } catch (AccessDeniedException | CommitFailedException e) {
+ LOG.debug("Failed to cache group membership: {}", e.getMessage());
Review Comment:
i think i would log that as a warning. not just on debug level.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
+ */
+class CachedGroupMembershipReader extends GroupMembershipReader {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class);
+
+ private static final long MEMBERSHIP_THRESHOLD = 0;
+
+ /**
+ * Keep track of cache updates for 100 most recent authorizables.
+ */
+ private static final int MAX_CACHE_TRACKING_ENTRIES = 100;
+
+ private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>();
+
+ private final UserConfiguration config;
+
+ private final Root root;
+
+ private final long expiration;
+ private final long maxStale;
+
+ CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory,
+ @NotNull UserConfiguration config,
+ @NotNull Root root) {
+ super(membershipProvider, groupPrincipalFactory);
+ this.config = checkNotNull(config);
+ this.root = root;
+ this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE);
+ this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE);
+ }
+
+ @Override
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ // membership cache only implemented on user
+ if (UserUtil.isType(authorizable, AuthorizableType.USER)) {
+ readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals);
+ } else {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+ }
+
+ private void readGroupsFromCache(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groups,
+ @NotNull BiConsumer<Tree, Set<Principal>> loader) {
+ long expirationTime = 0;
+ String authorizablePath = authorizableNode.getPath();
+ Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (principalCache.exists()) {
+ expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE);
+ }
+ long now = System.currentTimeMillis();
+ if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) {
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ // need to load or serve stale if allowed
+ boolean mayServeStale = now - expirationTime < maxStale;
+ boolean updateCache;
+ Map<String, Long> updates = getCacheUpdateMap();
+ synchronized (updates) {
+ // anyone else updating that entry already?
+ Long exp = updates.get(authorizablePath);
+ updateCache = exp == null || expirationTime > exp;
+ if (updateCache) {
+ // we do it
+ updates.put(authorizablePath, expirationTime);
+ }
+ }
+
+ if (!updateCache && mayServeStale) {
+ LOG.debug("Another thread is updating the cache and we may serve stale");
Review Comment:
see above about the usage of 'may'. i would change the phrasing of this log output to make it meaningful for someone that is not familiar with the inner working of the cache. so explicitly stating that potentially outdated values are returned.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
+ */
+class CachedGroupMembershipReader extends GroupMembershipReader {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class);
+
+ private static final long MEMBERSHIP_THRESHOLD = 0;
+
+ /**
+ * Keep track of cache updates for 100 most recent authorizables.
+ */
+ private static final int MAX_CACHE_TRACKING_ENTRIES = 100;
+
+ private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>();
+
+ private final UserConfiguration config;
+
+ private final Root root;
+
+ private final long expiration;
+ private final long maxStale;
+
+ CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory,
+ @NotNull UserConfiguration config,
+ @NotNull Root root) {
+ super(membershipProvider, groupPrincipalFactory);
+ this.config = checkNotNull(config);
+ this.root = root;
+ this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE);
+ this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE);
+ }
+
+ @Override
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ // membership cache only implemented on user
+ if (UserUtil.isType(authorizable, AuthorizableType.USER)) {
+ readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals);
+ } else {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+ }
+
+ private void readGroupsFromCache(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groups,
+ @NotNull BiConsumer<Tree, Set<Principal>> loader) {
+ long expirationTime = 0;
+ String authorizablePath = authorizableNode.getPath();
+ Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (principalCache.exists()) {
+ expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE);
+ }
+ long now = System.currentTimeMillis();
+ if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) {
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ // need to load or serve stale if allowed
+ boolean mayServeStale = now - expirationTime < maxStale;
+ boolean updateCache;
+ Map<String, Long> updates = getCacheUpdateMap();
+ synchronized (updates) {
+ // anyone else updating that entry already?
+ Long exp = updates.get(authorizablePath);
+ updateCache = exp == null || expirationTime > exp;
+ if (updateCache) {
+ // we do it
Review Comment:
we do what? please elaborate
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupMembershipReader.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.jetbrains.annotations.NotNull;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+
+/**
+ * <code>GroupMembershipReader</code>...
+ */
+class GroupMembershipReader {
+
+ private final MembershipProvider membershipProvider;
+
+ protected final GroupPrincipalFactory groupPrincipalFactory;
+
+ GroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory) {
+ this.membershipProvider = checkNotNull(membershipProvider);
+ this.groupPrincipalFactory = checkNotNull(groupPrincipalFactory);
+ }
+
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+
+ protected final void loadGroupPrincipals(@NotNull Tree authorizableTree,
+ @NotNull Set<Principal> groupPrincipals) {
+ Iterator<Tree> groupTrees = membershipProvider.getMembership(authorizableTree, true);
+ while (groupTrees.hasNext()) {
+ Tree groupTree = groupTrees.next();
+ if (UserUtil.isType(groupTree, AuthorizableType.GROUP)) {
+ Principal gr = groupPrincipalFactory.create(groupTree);
+ if (gr != null) {
+ groupPrincipals.add(gr);
+ }
+ }
+ }
+ }
+
+ interface GroupPrincipalFactory {
+
+ Principal create(@NotNull Tree authorizable);
+
+ Principal create(@NotNull String principalName);
Review Comment:
return value should be annotated with NotNull or Nullable
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
Review Comment:
either add proper javadoc or drop it. and again.... rather use {@code} and <code>
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
+ */
+class CachedGroupMembershipReader extends GroupMembershipReader {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class);
+
+ private static final long MEMBERSHIP_THRESHOLD = 0;
+
+ /**
+ * Keep track of cache updates for 100 most recent authorizables.
+ */
+ private static final int MAX_CACHE_TRACKING_ENTRIES = 100;
+
+ private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>();
+
+ private final UserConfiguration config;
+
+ private final Root root;
+
+ private final long expiration;
+ private final long maxStale;
+
+ CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory,
+ @NotNull UserConfiguration config,
+ @NotNull Root root) {
+ super(membershipProvider, groupPrincipalFactory);
+ this.config = checkNotNull(config);
+ this.root = root;
+ this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE);
+ this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE);
+ }
+
+ @Override
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ // membership cache only implemented on user
+ if (UserUtil.isType(authorizable, AuthorizableType.USER)) {
+ readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals);
+ } else {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+ }
+
+ private void readGroupsFromCache(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groups,
+ @NotNull BiConsumer<Tree, Set<Principal>> loader) {
+ long expirationTime = 0;
+ String authorizablePath = authorizableNode.getPath();
+ Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (principalCache.exists()) {
+ expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE);
+ }
+ long now = System.currentTimeMillis();
+ if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) {
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ // need to load or serve stale if allowed
+ boolean mayServeStale = now - expirationTime < maxStale;
+ boolean updateCache;
+ Map<String, Long> updates = getCacheUpdateMap();
+ synchronized (updates) {
+ // anyone else updating that entry already?
+ Long exp = updates.get(authorizablePath);
+ updateCache = exp == null || expirationTime > exp;
Review Comment:
i would find it easier to read with () around the or-ing expression: updateCache = (exp == null || expirationTime > exp). to have a visual distinction between the assignment and the subsequent '=='
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupMembershipReader.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.jetbrains.annotations.NotNull;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+
+/**
+ * <code>GroupMembershipReader</code>...
Review Comment:
this deserves a bit of javadoc. if we believe it's not neeeded drop it altogether.
apart from that: modern style javadoc uses {@code} instead of <code>... my IDE even complains about it :)
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
+ */
+class CachedGroupMembershipReader extends GroupMembershipReader {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class);
+
+ private static final long MEMBERSHIP_THRESHOLD = 0;
+
+ /**
+ * Keep track of cache updates for 100 most recent authorizables.
+ */
+ private static final int MAX_CACHE_TRACKING_ENTRIES = 100;
+
+ private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>();
+
+ private final UserConfiguration config;
+
+ private final Root root;
+
+ private final long expiration;
+ private final long maxStale;
+
+ CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory,
+ @NotNull UserConfiguration config,
+ @NotNull Root root) {
+ super(membershipProvider, groupPrincipalFactory);
+ this.config = checkNotNull(config);
+ this.root = root;
+ this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE);
+ this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE);
+ }
+
+ @Override
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ // membership cache only implemented on user
+ if (UserUtil.isType(authorizable, AuthorizableType.USER)) {
+ readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals);
+ } else {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+ }
+
+ private void readGroupsFromCache(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groups,
+ @NotNull BiConsumer<Tree, Set<Principal>> loader) {
+ long expirationTime = 0;
+ String authorizablePath = authorizableNode.getPath();
+ Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (principalCache.exists()) {
+ expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE);
+ }
+ long now = System.currentTimeMillis();
+ if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) {
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ // need to load or serve stale if allowed
+ boolean mayServeStale = now - expirationTime < maxStale;
Review Comment:
calculation of mayServeStale can be moved down closer to the if where it is evaluated such that i don't have to scroll up again to check.
also: why is it 'may'? if that variable is true and updating the cache was not possible, we do return a stale cache, no? so may sounds wrong to me.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupMembershipReader.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.Iterator;
+import java.util.Set;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.jetbrains.annotations.NotNull;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+
+/**
+ * <code>GroupMembershipReader</code>...
+ */
+class GroupMembershipReader {
+
+ private final MembershipProvider membershipProvider;
+
+ protected final GroupPrincipalFactory groupPrincipalFactory;
Review Comment:
why is this protected? i don't see any need for a sub-class outside of this very package that would need to access this field. in fact i would prefer to have the subclass access it through a package-accessible method.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) {
}
//------------------------------------------------------------< private >---
+
+ GroupPrincipalFactory createGroupPrincipalFactory() {
Review Comment:
return value should be annotated with @NotNull
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
+ */
+class CachedGroupMembershipReader extends GroupMembershipReader {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class);
+
+ private static final long MEMBERSHIP_THRESHOLD = 0;
+
+ /**
+ * Keep track of cache updates for 100 most recent authorizables.
+ */
+ private static final int MAX_CACHE_TRACKING_ENTRIES = 100;
+
+ private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>();
+
+ private final UserConfiguration config;
+
+ private final Root root;
+
+ private final long expiration;
+ private final long maxStale;
+
+ CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory,
+ @NotNull UserConfiguration config,
+ @NotNull Root root) {
+ super(membershipProvider, groupPrincipalFactory);
+ this.config = checkNotNull(config);
+ this.root = root;
+ this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE);
+ this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE);
+ }
+
+ @Override
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ // membership cache only implemented on user
+ if (UserUtil.isType(authorizable, AuthorizableType.USER)) {
+ readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals);
+ } else {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+ }
+
+ private void readGroupsFromCache(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groups,
+ @NotNull BiConsumer<Tree, Set<Principal>> loader) {
+ long expirationTime = 0;
+ String authorizablePath = authorizableNode.getPath();
+ Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (principalCache.exists()) {
+ expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE);
+ }
+ long now = System.currentTimeMillis();
+ if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) {
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ // need to load or serve stale if allowed
+ boolean mayServeStale = now - expirationTime < maxStale;
+ boolean updateCache;
+ Map<String, Long> updates = getCacheUpdateMap();
+ synchronized (updates) {
+ // anyone else updating that entry already?
+ Long exp = updates.get(authorizablePath);
+ updateCache = exp == null || expirationTime > exp;
+ if (updateCache) {
+ // we do it
+ updates.put(authorizablePath, expirationTime);
+ }
+ }
+
+ if (!updateCache && mayServeStale) {
+ LOG.debug("Another thread is updating the cache and we may serve stale");
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ if (updateCache) {
+ boolean cacheSuccess = false;
+ try {
+ loader.accept(authorizableNode, groups);
+ cacheGroups(authorizableNode, groups);
+ cacheSuccess = true;
+ } catch (AccessDeniedException | CommitFailedException e) {
+ LOG.debug("Failed to cache group membership: {}", e.getMessage());
+ } finally {
+ if (!cacheSuccess || expirationTime == 0) {
+ synchronized (updates) {
+ Long exp = updates.get(authorizablePath);
+ if (Objects.equals(exp, expirationTime)) {
+ updates.remove(authorizablePath);
+ }
+ }
+ }
+ }
+ } else {
+ // load but do not cache. happens when another thread is updating
+ // the cache and this thread is not allowed to serve stale
+ LOG.debug("Load but do not cache. Another thread is updating the cache and this thread is not allowed to serve stale");
Review Comment:
same applies here. if i were an outsider i would not understand what is 'stale' and what the implications are.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) {
}
//------------------------------------------------------------< private >---
+
+ GroupPrincipalFactory createGroupPrincipalFactory() {
Review Comment:
missing annotation of return value (adding notnull would be the correct one afaik).
i am wondering why this method is not private. IMHO nobody outside this class should use it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni closed pull request #1128: OAK-10451 UserPrincipalProvider may cause many conflicts when under load
URL: https://github.com/apache/jackrabbit-oak/pull/1128
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#issuecomment-2142532334
@nscendoni , i had another look at the patch and tried to address the TODO. lets set aside some time to go over the changes again, review if my updates are reasonable, working and make the code readable such that everyone would be able to maintain it in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1381960961
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
+ */
+class CachedGroupMembershipReader extends GroupMembershipReader {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class);
+
+ private static final long MEMBERSHIP_THRESHOLD = 0;
+
+ /**
+ * Keep track of cache updates for 100 most recent authorizables.
+ */
+ private static final int MAX_CACHE_TRACKING_ENTRIES = 100;
+
+ private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>();
+
+ private final UserConfiguration config;
+
+ private final Root root;
+
+ private final long expiration;
+ private final long maxStale;
+
+ CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory,
+ @NotNull UserConfiguration config,
+ @NotNull Root root) {
+ super(membershipProvider, groupPrincipalFactory);
+ this.config = checkNotNull(config);
+ this.root = root;
+ this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE);
+ this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE);
+ }
+
+ @Override
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ // membership cache only implemented on user
+ if (UserUtil.isType(authorizable, AuthorizableType.USER)) {
+ readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals);
+ } else {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+ }
+
+ private void readGroupsFromCache(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groups,
+ @NotNull BiConsumer<Tree, Set<Principal>> loader) {
+ long expirationTime = 0;
+ String authorizablePath = authorizableNode.getPath();
+ Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (principalCache.exists()) {
+ expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE);
+ }
+ long now = System.currentTimeMillis();
+ if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) {
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ // need to load or serve stale if allowed
+ boolean mayServeStale = now - expirationTime < maxStale;
+ boolean updateCache;
+ Map<String, Long> updates = getCacheUpdateMap();
+ synchronized (updates) {
+ // anyone else updating that entry already?
+ Long exp = updates.get(authorizablePath);
+ updateCache = exp == null || expirationTime > exp;
+ if (updateCache) {
+ // we do it
+ updates.put(authorizablePath, expirationTime);
+ }
+ }
+
+ if (!updateCache && mayServeStale) {
+ LOG.debug("Another thread is updating the cache and we may serve stale");
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ if (updateCache) {
+ boolean cacheSuccess = false;
+ try {
+ loader.accept(authorizableNode, groups);
+ cacheGroups(authorizableNode, groups);
+ cacheSuccess = true;
+ } catch (AccessDeniedException | CommitFailedException e) {
+ LOG.debug("Failed to cache group membership: {}", e.getMessage());
+ } finally {
+ if (!cacheSuccess || expirationTime == 0) {
+ synchronized (updates) {
+ Long exp = updates.get(authorizablePath);
+ if (Objects.equals(exp, expirationTime)) {
+ updates.remove(authorizablePath);
+ }
+ }
+ }
+ }
+ } else {
+ // load but do not cache. happens when another thread is updating
+ // the cache and this thread is not allowed to serve stale
+ LOG.debug("Load but do not cache. Another thread is updating the cache and this thread is not allowed to serve stale");
+ loader.accept(authorizableNode, groups);
+ }
+ }
+
+ private void serveGroupsFromCache(String authorizablePath,
+ Tree principalCache,
+ Set<Principal> groups) {
+ LOG.debug("Reading group membership at {}", authorizablePath);
+
+ String str = TreeUtil.getString(principalCache, REP_GROUP_PRINCIPAL_NAMES);
+ if (Strings.isNullOrEmpty(str)) {
+ return;
+ }
+
+ for (String s : Text.explode(str, ',')) {
+ final String name = Text.unescape(s);
+ groups.add(groupPrincipalFactory.create(name));
+ }
+ }
+
+ private void cacheGroups(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groupPrincipals)
+ throws AccessDeniedException, CommitFailedException {
+ try {
+ root.refresh();
+ Tree cache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (!cache.exists()) {
+ if (groupPrincipals.size() <= MEMBERSHIP_THRESHOLD) {
+ LOG.debug("Omit cache creation for user without group membership at {}", authorizableNode.getPath());
+ return;
+ } else {
+ LOG.debug("Create new group membership cache at {}", authorizableNode.getPath());
+ cache = TreeUtil.addChild(authorizableNode, CacheConstants.REP_CACHE, CacheConstants.NT_REP_CACHE);
+ }
+ }
+
+ cache.setProperty(CacheConstants.REP_EXPIRATION, LongUtils.calculateExpirationTime(expiration));
+ String value = (groupPrincipals.isEmpty()) ? "" : Joiner.on(",").join(Iterables.transform(groupPrincipals, input -> Text.escape(input.getName())));
+ cache.setProperty(REP_GROUP_PRINCIPAL_NAMES, value);
+
+ root.commit(CacheValidatorProvider.asCommitAttributes());
+ LOG.debug("Cached group membership at {}", authorizableNode.getPath());
+ } finally {
+ root.refresh();
+ }
+ }
+
+ private Map<String, Long> getCacheUpdateMap() {
+ return CACHE_UPDATES.computeIfAbsent(config, cfg -> new LinkedHashMap<>(){
Review Comment:
@mreutegg and @nscendoni , had another close look at the PR as i wanted to provide fixes for my picky comments myself.
and while doing that and touching the code in my IDE i realized that i don't understand the nature of the CACHE_UPDATES map that maps UserConfiguration to a second Map<String, Long> which marks the currently being updated authorizable-paths.
Therefore:
Unless i am really mistaken there exists only 1 UserConfiguration for a given oak repository as the SecurityProvider holds only one user configuration. do we need a map with key UserConfiguration?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "mreutegg (via GitHub)" <gi...@apache.org>.
mreutegg commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1382872642
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
+ */
+class CachedGroupMembershipReader extends GroupMembershipReader {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class);
+
+ private static final long MEMBERSHIP_THRESHOLD = 0;
+
+ /**
+ * Keep track of cache updates for 100 most recent authorizables.
+ */
+ private static final int MAX_CACHE_TRACKING_ENTRIES = 100;
+
+ private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>();
+
+ private final UserConfiguration config;
+
+ private final Root root;
+
+ private final long expiration;
+ private final long maxStale;
+
+ CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory,
+ @NotNull UserConfiguration config,
+ @NotNull Root root) {
+ super(membershipProvider, groupPrincipalFactory);
+ this.config = checkNotNull(config);
+ this.root = root;
+ this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE);
+ this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE);
+ }
+
+ @Override
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ // membership cache only implemented on user
+ if (UserUtil.isType(authorizable, AuthorizableType.USER)) {
+ readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals);
+ } else {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+ }
+
+ private void readGroupsFromCache(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groups,
+ @NotNull BiConsumer<Tree, Set<Principal>> loader) {
+ long expirationTime = 0;
+ String authorizablePath = authorizableNode.getPath();
+ Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (principalCache.exists()) {
+ expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE);
+ }
+ long now = System.currentTimeMillis();
+ if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) {
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ // need to load or serve stale if allowed
+ boolean mayServeStale = now - expirationTime < maxStale;
+ boolean updateCache;
+ Map<String, Long> updates = getCacheUpdateMap();
+ synchronized (updates) {
+ // anyone else updating that entry already?
+ Long exp = updates.get(authorizablePath);
+ updateCache = exp == null || expirationTime > exp;
+ if (updateCache) {
+ // we do it
+ updates.put(authorizablePath, expirationTime);
+ }
+ }
+
+ if (!updateCache && mayServeStale) {
+ LOG.debug("Another thread is updating the cache and we may serve stale");
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ if (updateCache) {
+ boolean cacheSuccess = false;
+ try {
+ loader.accept(authorizableNode, groups);
+ cacheGroups(authorizableNode, groups);
+ cacheSuccess = true;
+ } catch (AccessDeniedException | CommitFailedException e) {
+ LOG.debug("Failed to cache group membership: {}", e.getMessage());
+ } finally {
+ if (!cacheSuccess || expirationTime == 0) {
+ synchronized (updates) {
+ Long exp = updates.get(authorizablePath);
+ if (Objects.equals(exp, expirationTime)) {
+ updates.remove(authorizablePath);
+ }
+ }
+ }
+ }
+ } else {
+ // load but do not cache. happens when another thread is updating
+ // the cache and this thread is not allowed to serve stale
+ LOG.debug("Load but do not cache. Another thread is updating the cache and this thread is not allowed to serve stale");
+ loader.accept(authorizableNode, groups);
+ }
+ }
+
+ private void serveGroupsFromCache(String authorizablePath,
+ Tree principalCache,
+ Set<Principal> groups) {
+ LOG.debug("Reading group membership at {}", authorizablePath);
+
+ String str = TreeUtil.getString(principalCache, REP_GROUP_PRINCIPAL_NAMES);
+ if (Strings.isNullOrEmpty(str)) {
+ return;
+ }
+
+ for (String s : Text.explode(str, ',')) {
+ final String name = Text.unescape(s);
+ groups.add(groupPrincipalFactory.create(name));
+ }
+ }
+
+ private void cacheGroups(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groupPrincipals)
+ throws AccessDeniedException, CommitFailedException {
+ try {
+ root.refresh();
+ Tree cache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (!cache.exists()) {
+ if (groupPrincipals.size() <= MEMBERSHIP_THRESHOLD) {
+ LOG.debug("Omit cache creation for user without group membership at {}", authorizableNode.getPath());
+ return;
+ } else {
+ LOG.debug("Create new group membership cache at {}", authorizableNode.getPath());
+ cache = TreeUtil.addChild(authorizableNode, CacheConstants.REP_CACHE, CacheConstants.NT_REP_CACHE);
+ }
+ }
+
+ cache.setProperty(CacheConstants.REP_EXPIRATION, LongUtils.calculateExpirationTime(expiration));
+ String value = (groupPrincipals.isEmpty()) ? "" : Joiner.on(",").join(Iterables.transform(groupPrincipals, input -> Text.escape(input.getName())));
+ cache.setProperty(REP_GROUP_PRINCIPAL_NAMES, value);
+
+ root.commit(CacheValidatorProvider.asCommitAttributes());
+ LOG.debug("Cached group membership at {}", authorizableNode.getPath());
+ } finally {
+ root.refresh();
+ }
+ }
+
+ private Map<String, Long> getCacheUpdateMap() {
+ return CACHE_UPDATES.computeIfAbsent(config, cfg -> new LinkedHashMap<>(){
Review Comment:
It ensures the cache is per repository and not shared between multiple repository instances. I guess, a better solution would directly tie the cache to a repository instance and pass it down to the CachedGroupMembershipReader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1371646420
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) {
}
//------------------------------------------------------------< private >---
+
+ GroupPrincipalFactory createGroupPrincipalFactory() {
Review Comment:
the return value should be annotated with '@NotNull' and the methods should be private. IMHO nobody outside this class needs to access it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1383684011
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
+ */
+class CachedGroupMembershipReader extends GroupMembershipReader {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class);
+
+ private static final long MEMBERSHIP_THRESHOLD = 0;
+
+ /**
+ * Keep track of cache updates for 100 most recent authorizables.
+ */
+ private static final int MAX_CACHE_TRACKING_ENTRIES = 100;
+
+ private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>();
+
+ private final UserConfiguration config;
+
+ private final Root root;
+
+ private final long expiration;
+ private final long maxStale;
+
+ CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider,
+ @NotNull GroupPrincipalFactory groupPrincipalFactory,
+ @NotNull UserConfiguration config,
+ @NotNull Root root) {
+ super(membershipProvider, groupPrincipalFactory);
+ this.config = checkNotNull(config);
+ this.root = root;
+ this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE);
+ this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE);
+ }
+
+ @Override
+ void getMembership(@NotNull Tree authorizable,
+ @NotNull Set<Principal> groupPrincipals) {
+ // membership cache only implemented on user
+ if (UserUtil.isType(authorizable, AuthorizableType.USER)) {
+ readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals);
+ } else {
+ loadGroupPrincipals(authorizable, groupPrincipals);
+ }
+ }
+
+ private void readGroupsFromCache(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groups,
+ @NotNull BiConsumer<Tree, Set<Principal>> loader) {
+ long expirationTime = 0;
+ String authorizablePath = authorizableNode.getPath();
+ Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (principalCache.exists()) {
+ expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE);
+ }
+ long now = System.currentTimeMillis();
+ if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) {
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ // need to load or serve stale if allowed
+ boolean mayServeStale = now - expirationTime < maxStale;
+ boolean updateCache;
+ Map<String, Long> updates = getCacheUpdateMap();
+ synchronized (updates) {
+ // anyone else updating that entry already?
+ Long exp = updates.get(authorizablePath);
+ updateCache = exp == null || expirationTime > exp;
+ if (updateCache) {
+ // we do it
+ updates.put(authorizablePath, expirationTime);
+ }
+ }
+
+ if (!updateCache && mayServeStale) {
+ LOG.debug("Another thread is updating the cache and we may serve stale");
+ serveGroupsFromCache(authorizablePath, principalCache, groups);
+ return;
+ }
+
+ if (updateCache) {
+ boolean cacheSuccess = false;
+ try {
+ loader.accept(authorizableNode, groups);
+ cacheGroups(authorizableNode, groups);
+ cacheSuccess = true;
+ } catch (AccessDeniedException | CommitFailedException e) {
+ LOG.debug("Failed to cache group membership: {}", e.getMessage());
+ } finally {
+ if (!cacheSuccess || expirationTime == 0) {
+ synchronized (updates) {
+ Long exp = updates.get(authorizablePath);
+ if (Objects.equals(exp, expirationTime)) {
+ updates.remove(authorizablePath);
+ }
+ }
+ }
+ }
+ } else {
+ // load but do not cache. happens when another thread is updating
+ // the cache and this thread is not allowed to serve stale
+ LOG.debug("Load but do not cache. Another thread is updating the cache and this thread is not allowed to serve stale");
+ loader.accept(authorizableNode, groups);
+ }
+ }
+
+ private void serveGroupsFromCache(String authorizablePath,
+ Tree principalCache,
+ Set<Principal> groups) {
+ LOG.debug("Reading group membership at {}", authorizablePath);
+
+ String str = TreeUtil.getString(principalCache, REP_GROUP_PRINCIPAL_NAMES);
+ if (Strings.isNullOrEmpty(str)) {
+ return;
+ }
+
+ for (String s : Text.explode(str, ',')) {
+ final String name = Text.unescape(s);
+ groups.add(groupPrincipalFactory.create(name));
+ }
+ }
+
+ private void cacheGroups(@NotNull Tree authorizableNode,
+ @NotNull Set<Principal> groupPrincipals)
+ throws AccessDeniedException, CommitFailedException {
+ try {
+ root.refresh();
+ Tree cache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (!cache.exists()) {
+ if (groupPrincipals.size() <= MEMBERSHIP_THRESHOLD) {
+ LOG.debug("Omit cache creation for user without group membership at {}", authorizableNode.getPath());
+ return;
+ } else {
+ LOG.debug("Create new group membership cache at {}", authorizableNode.getPath());
+ cache = TreeUtil.addChild(authorizableNode, CacheConstants.REP_CACHE, CacheConstants.NT_REP_CACHE);
+ }
+ }
+
+ cache.setProperty(CacheConstants.REP_EXPIRATION, LongUtils.calculateExpirationTime(expiration));
+ String value = (groupPrincipals.isEmpty()) ? "" : Joiner.on(",").join(Iterables.transform(groupPrincipals, input -> Text.escape(input.getName())));
+ cache.setProperty(REP_GROUP_PRINCIPAL_NAMES, value);
+
+ root.commit(CacheValidatorProvider.asCommitAttributes());
+ LOG.debug("Cached group membership at {}", authorizableNode.getPath());
+ } finally {
+ root.refresh();
+ }
+ }
+
+ private Map<String, Long> getCacheUpdateMap() {
+ return CACHE_UPDATES.computeIfAbsent(config, cfg -> new LinkedHashMap<>(){
Review Comment:
hmmmm.... i don't like the idea of leaking out implementation details of the user management to the whole repository setup. i would like to be able to potentially replace that without having to care about the rest of oak.
so, i will leave it as it is and add a comment explaining the code such that the next reader will understand the intention..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela merged PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1371686412
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import java.security.Principal;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import javax.jcr.AccessDeniedException;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.base.Strings;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull;
+import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION;
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE;
+
+/**
+ * <code>CachedGroupMembershipReader</code>...
Review Comment:
either add proper javadoc or drop it. and again.... rather use` {@code}` instead of` <code>`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1371690609
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) {
}
//------------------------------------------------------------< private >---
+
+ GroupPrincipalFactory createGroupPrincipalFactory() {
Review Comment:
missing annotation of return value (adding notnull would be the correct one afaik).
i am wondering why this method is not private. IMHO nobody outside this class should use it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1356466127
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheCommitterThread.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.jcr.AccessDeniedException;
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+class PrincipalCacheCommitterThread extends Thread {
+ protected static final long MEMBERSHIP_THRESHOLD = 0;
Review Comment:
this constant can be private as it is only used here, right?
##########
oak-benchmarks/LoginTest_20231010_172626.csv:
##########
Review Comment:
we don't need to have the benchmark results committed to git. having them in jira for the record is sufficient.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheCommitterThread.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.util.Text;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.jcr.AccessDeniedException;
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+class PrincipalCacheCommitterThread extends Thread {
+ protected static final long MEMBERSHIP_THRESHOLD = 0;
+ private final Tree authorizableNode;
+ private final Set<Principal> groupPrincipals;
+ private final long expiration;
+ private final Root root;
+
+ private static final Logger log = LoggerFactory.getLogger(PrincipalCacheCommitterThread.class);
+
+ protected PrincipalCacheCommitterThread(@NotNull Tree authorizableNode, @NotNull Set<Principal> groupPrincipals, long expiration, @NotNull Root root) {
Review Comment:
protected can be omitted as it is only used inside this package
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CacheThreadProvider.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+class CacheThreadProvider {
+
+ private static CacheThreadProvider instance = null;
+ private static ConcurrentHashMap<String, PrincipalCacheCommitterThread> committerThreadMap = new ConcurrentHashMap<>();
+
+ public static @Nullable CacheThreadProvider getInstance() {
+ if (instance == null) {
+ instance = new CacheThreadProvider();
+ }
+ return instance;
+ }
+
+ protected synchronized @Nullable PrincipalCacheCommitterThread cacheGroups(@NotNull Tree authorizableNode, @NotNull Set<Principal> groupPrincipals, long expiration, @NotNull Root root) {
Review Comment:
'protected' can be omitted making the intention clear that this class is only visible and accessible inside this package.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CacheThreadProvider.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+class CacheThreadProvider {
+
+ private static CacheThreadProvider instance = null;
+ private static ConcurrentHashMap<String, PrincipalCacheCommitterThread> committerThreadMap = new ConcurrentHashMap<>();
+
+ public static @Nullable CacheThreadProvider getInstance() {
+ if (instance == null) {
+ instance = new CacheThreadProvider();
+ }
+ return instance;
+ }
+
+ protected synchronized @Nullable PrincipalCacheCommitterThread cacheGroups(@NotNull Tree authorizableNode, @NotNull Set<Principal> groupPrincipals, long expiration, @NotNull Root root) {
+ String authorizableNodePath = authorizableNode.getPath();
+ if (committerThreadMap.containsKey(authorizableNodePath)) {
+ // One thread is already committing. return null to inform the caller that doesn't have to wait for the commit to finish
+ return null;
+ } else {
+ PrincipalCacheCommitterThread committerThread = new PrincipalCacheCommitterThread(authorizableNode, groupPrincipals, expiration, root);
+ committerThreadMap.put(authorizableNodePath, committerThread);
+ committerThread.start();
+ return committerThread;
+ }
+ }
+
+ protected ConcurrentHashMap<String, PrincipalCacheCommitterThread> getCommitterThreadMap() {
Review Comment:
this method is only used in tests.
for the sake of clarity i would rather remove this method and use reflection to look at the map in tests.
if you prefer to keep it,
- please remove 'protected' as this should not be needed
- annotate with 'nonnull'
- add a comment that this is used in tests only.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CacheThreadProvider.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+class CacheThreadProvider {
+
+ private static CacheThreadProvider instance = null;
+ private static ConcurrentHashMap<String, PrincipalCacheCommitterThread> committerThreadMap = new ConcurrentHashMap<>();
+
+ public static @Nullable CacheThreadProvider getInstance() {
Review Comment:
this method always returns a non-null value... ->` @NotNull` annotation.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CacheThreadProvider.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+class CacheThreadProvider {
+
+ private static CacheThreadProvider instance = null;
Review Comment:
i would recommend to create the instance here and mark it as private static final and then just return it upon getInstance().
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -287,7 +280,16 @@ private Set<Principal> getGroupMembership(@NotNull Tree authorizableTree) {
// remember the regular groups in case caching is enabled
if (doCache) {
- cacheGroups(authorizableTree, groupPrincipals);
+ PrincipalCacheCommitterThread commitThread = CacheThreadProvider.getInstance().cacheGroups(authorizableTree, groupPrincipals, expiration, root);
Review Comment:
the 'commitThread' variable can just be of type Thread.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CacheThreadProvider.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+class CacheThreadProvider {
+
+ private static CacheThreadProvider instance = null;
+ private static ConcurrentHashMap<String, PrincipalCacheCommitterThread> committerThreadMap = new ConcurrentHashMap<>();
+
+ public static @Nullable CacheThreadProvider getInstance() {
+ if (instance == null) {
Review Comment:
see above. just return instance and avoid creating it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1349992660
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
+ * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li>
+ * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li>
+ * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li>
+ * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li>
+ * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li>
+ * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li>
+ * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li>
+ * </ul>
+ */
+
+public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler {
+
+ private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class);
+
+ protected static final String REP_EXPIRATION = "rep:expiration";
+ protected static final String REP_CACHE = "rep:cache";
+
+ /**
+ * Create a new {@code ConflictHandler} which always returns
+ * {@code resolution}.
+ *
+ * @param resolution the resolution to return from all methods of this
+ * {@code ConflictHandler} instance.
+ */
+ public PrincipalCacheConflictHandler(Resolution resolution) {
+ super(resolution);
+ }
+ public PrincipalCacheConflictHandler() {
+ this(Resolution.IGNORED);
+ }
+
+ private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs,
+ PropertyState base) {
+ if ( REP_EXPIRATION.equals(ours.getName()) && REP_EXPIRATION.equals(theirs.getName()) ){
+
+ PropertyBuilder<Long> merged = PropertyBuilder.scalar(Type.LONG);
+ merged.setName(REP_EXPIRATION);
+
+ //if base is bigger than ours and theirs, then use base. This should never happens
+ if ( base != null &&
+ base.getValue(Type.LONG) > ours.getValue(Type.LONG) &&
+ base.getValue(Type.LONG) > theirs.getValue(Type.LONG) ){
+ merged.setValue(base.getValue(Type.LONG));
+ return Resolution.MERGED;
+ }
+
+ //if ours is bigger than theirs, then use ours
+ //otherwise use theirs
+ if ( ours.getValue(Type.LONG) > theirs.getValue(Type.LONG) ){
+ merged.setValue(ours.getValue(Type.LONG));
+ } else {
+ merged.setValue(theirs.getValue(Type.LONG));
+ }
+ parent.setProperty(merged.getPropertyState());
+ LOG.debug("Resolved conflict for property {} our value: {}, merged value: {}", REP_EXPIRATION, ours.getValue(Type.LONG), merged.getValue(0));
+ return Resolution.MERGED;
+ }
+ return Resolution.IGNORED;
+
+ }
+
+ @Override
+ public Resolution changeChangedProperty(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs,
+ @NotNull PropertyState base) {
+
+ return resolveRepExpirationConflict(parent, ours, theirs, base);
Review Comment:
In the ticket were reported only issues with PrincipalCache's rep:expiration time. I would create a different ticket for rep:principalsNames if required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#issuecomment-1792797994
@nscendoni , i attempted to fix my picky review findings. would you have time to check if those are ok with you and also if the comments i added/modified are correctly describing the intention of the code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1346059752
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
+ * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li>
+ * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li>
+ * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li>
+ * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li>
+ * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li>
+ * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li>
+ * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li>
+ * </ul>
+ */
+
+public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler {
+
+ private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class);
+
+ protected static final String REP_EXPIRATION = "rep:expiration";
+ protected static final String REP_CACHE = "rep:cache";
Review Comment:
duplicate constant -> i believe this can be replace by implementing CacheConstants
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
+ * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li>
+ * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li>
+ * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li>
+ * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li>
+ * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li>
+ * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li>
+ * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li>
+ * </ul>
+ */
+
+public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler {
Review Comment:
the conflict-handler can be package protected, right? it's only referenced by the UserConfigurationImpl and thus doesn't need to be public.
i would also consider if CacheConflictHandler would be sufficient accurate as a name as the other classes related to the principal-cache are name 'CacheConstants' and 'CacheValidatorProvider'. WDYT?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
+ * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li>
+ * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li>
+ * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li>
+ * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li>
+ * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li>
+ * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li>
+ * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li>
+ * </ul>
+ */
+
+public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler {
+
+ private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class);
+
+ protected static final String REP_EXPIRATION = "rep:expiration";
Review Comment:
duplicate constant -> i believe this can be replace by implementing CacheConstants
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
Review Comment:
typo in 'conflints'
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -75,7 +75,7 @@ class UserPrincipalProvider implements PrincipalProvider {
static final String PARAM_CACHE_EXPIRATION = "cacheExpiration";
static final long EXPIRATION_NO_CACHE = 0;
- private static final long MEMBERSHIP_THRESHOLD = 0;
+ protected static final long MEMBERSHIP_THRESHOLD = 0;
Review Comment:
instead of making the constant protected -> move it to `PrincipalCommitterThread` as it is no longer used here in the `UserPrincipalProvider`
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+public class UserPrincipalProviderCommitterProvider {
+
+ static UserPrincipalProviderCommitterProvider instance = null;
+ static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>();
+
+ public static UserPrincipalProviderCommitterProvider getInstance() {
+ if (instance == null) {
+ instance = new UserPrincipalProviderCommitterProvider();
+ }
+ return instance;
+ }
+
+ public synchronized PrincipalCommitterThread cacheGroups(@NotNull Tree authorizableNode, @NotNull Set<Principal> groupPrincipals, long expiration, Root root) {
Review Comment:
- return value annotation with `@Nullable`
- Root should be annotated with `@NotNull`
- the method doesn't need to be public as it is only consumed by the userprincpialprovider -> package private
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+public class UserPrincipalProviderCommitterProvider {
+
+ static UserPrincipalProviderCommitterProvider instance = null;
+ static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>();
+
+ public static UserPrincipalProviderCommitterProvider getInstance() {
Review Comment:
return value annotation with `@Nullable`
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/AbstractSecurityTest.java:
##########
@@ -89,8 +89,8 @@ public abstract class AbstractSecurityTest {
protected Root root;
protected QueryEngineSettings querySettings;
- private final RootProvider rootProvider = new RootProviderService();
- private final TreeProvider treeProvider = new TreeProviderService();
+ protected final RootProvider rootProvider = new RootProviderService();
+ protected final TreeProvider treeProvider = new TreeProviderService();
Review Comment:
please use getTreeProvider() in the conflict-handler test instead of altering the access modifier.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+public class UserPrincipalProviderCommitterProvider {
+
+ static UserPrincipalProviderCommitterProvider instance = null;
+ static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>();
+
+ public static UserPrincipalProviderCommitterProvider getInstance() {
+ if (instance == null) {
+ instance = new UserPrincipalProviderCommitterProvider();
+ }
+ return instance;
+ }
+
+ public synchronized PrincipalCommitterThread cacheGroups(@NotNull Tree authorizableNode, @NotNull Set<Principal> groupPrincipals, long expiration, Root root) {
+ String authorizableNodePath = authorizableNode.getPath();
+ if (committerThreadMap.containsKey(authorizableNodePath)) {
+ // One thread is already committing. return null to inform the caller that doesn't have to wait for the commit to finish
+ return null;
+ } else {
+ PrincipalCommitterThread committerThread = new PrincipalCommitterThread(authorizableNode, groupPrincipals, expiration, root, committerThreadMap);
Review Comment:
see above regarding passing the thread-map around and have the removal in another class.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCommitterThread.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.util.Text;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.jcr.AccessDeniedException;
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.MEMBERSHIP_THRESHOLD;
+
+public class PrincipalCommitterThread extends Thread {
+ Tree authorizableNode;
+ Set<Principal> groupPrincipals;
+ HashMap committerThreadMap;
+ long expiration;
+ Root root;
+
+ private static final Logger log = LoggerFactory.getLogger(PrincipalCommitterThread.class);
+
+ public PrincipalCommitterThread(Tree authorizableNode, Set<Principal> groupPrincipals, long expiration, Root root, HashMap committerThreadMap) {
+ this.authorizableNode = authorizableNode;
+ this.groupPrincipals = groupPrincipals;
+ this.committerThreadMap = committerThreadMap;
+ this.expiration = expiration;
+ this.root = root;
+ }
+
+ @Override
+ public void run() {
+ super.run();
+ // Do the commit
+ try {
+ root.refresh();
+
+ Tree cache = authorizableNode.getChild(CacheConstants.REP_CACHE);
+ if (!cache.exists()) {
+ if (groupPrincipals.size() <= MEMBERSHIP_THRESHOLD) {
+ log.debug("Omit cache creation for user without group membership at {}", authorizableNode.getPath());
+ return;
+ } else {
+ log.debug("Create new group membership cache at {}", authorizableNode.getPath());
+ cache = TreeUtil.addChild(authorizableNode, CacheConstants.REP_CACHE, CacheConstants.NT_REP_CACHE);
+ }
+ }
+
+ cache.setProperty(CacheConstants.REP_EXPIRATION, LongUtils.calculateExpirationTime(expiration));
+ String value = (groupPrincipals.isEmpty()) ? "" : Joiner.on(",").join(Iterables.transform(groupPrincipals, input -> Text.escape(input.getName())));
+ cache.setProperty(CacheConstants.REP_GROUP_PRINCIPAL_NAMES, value);
+
+ root.commit(CacheValidatorProvider.asCommitAttributes());
+ log.debug("Cached group membership at {}", authorizableNode.getPath());
+
+ } catch (AccessDeniedException | CommitFailedException e) {
+ log.debug("Failed to cache group membership: {}", e.getMessage());
+ } finally {
+ log.debug("Removing thread from committerThreadMap for {}", authorizableNode.getPath());
+ committerThreadMap.remove(authorizableNode.getPath());
Review Comment:
see comments elsewhere.... that looks troublesome to me.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
+ * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li>
+ * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li>
+ * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li>
+ * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li>
+ * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li>
+ * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li>
+ * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li>
+ * </ul>
+ */
+
+public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler {
+
+ private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class);
+
+ protected static final String REP_EXPIRATION = "rep:expiration";
+ protected static final String REP_CACHE = "rep:cache";
+
+ /**
+ * Create a new {@code ConflictHandler} which always returns
+ * {@code resolution}.
+ *
+ * @param resolution the resolution to return from all methods of this
+ * {@code ConflictHandler} instance.
+ */
+ public PrincipalCacheConflictHandler(Resolution resolution) {
Review Comment:
as far as i can see this constructor is only used by the second constructor. so, i would suggest to either make it private or drop it altogether and simplify the code.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
+ * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li>
+ * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li>
+ * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li>
+ * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li>
+ * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li>
+ * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li>
+ * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li>
+ * </ul>
+ */
+
+public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler {
+
+ private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class);
+
+ protected static final String REP_EXPIRATION = "rep:expiration";
+ protected static final String REP_CACHE = "rep:cache";
+
+ /**
+ * Create a new {@code ConflictHandler} which always returns
+ * {@code resolution}.
+ *
+ * @param resolution the resolution to return from all methods of this
+ * {@code ConflictHandler} instance.
+ */
+ public PrincipalCacheConflictHandler(Resolution resolution) {
+ super(resolution);
+ }
+ public PrincipalCacheConflictHandler() {
+ this(Resolution.IGNORED);
Review Comment:
see above. maybe just calling super(Resolution.IGNORED) and drop the first constructor.
then, the constructor does not need to be public IMHO. i would recommend to make package protected...
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
+ * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li>
+ * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li>
+ * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li>
+ * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li>
+ * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li>
+ * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li>
+ * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li>
+ * </ul>
+ */
+
+public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler {
+
+ private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class);
+
+ protected static final String REP_EXPIRATION = "rep:expiration";
+ protected static final String REP_CACHE = "rep:cache";
+
+ /**
+ * Create a new {@code ConflictHandler} which always returns
+ * {@code resolution}.
+ *
+ * @param resolution the resolution to return from all methods of this
+ * {@code ConflictHandler} instance.
+ */
+ public PrincipalCacheConflictHandler(Resolution resolution) {
+ super(resolution);
+ }
+ public PrincipalCacheConflictHandler() {
+ this(Resolution.IGNORED);
+ }
+
+ private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs,
+ PropertyState base) {
+ if ( REP_EXPIRATION.equals(ours.getName()) && REP_EXPIRATION.equals(theirs.getName()) ){
+
+ PropertyBuilder<Long> merged = PropertyBuilder.scalar(Type.LONG);
+ merged.setName(REP_EXPIRATION);
+
+ //if base is bigger than ours and theirs, then use base. This should never happens
+ if ( base != null &&
+ base.getValue(Type.LONG) > ours.getValue(Type.LONG) &&
+ base.getValue(Type.LONG) > theirs.getValue(Type.LONG) ){
+ merged.setValue(base.getValue(Type.LONG));
+ return Resolution.MERGED;
+ }
+
+ //if ours is bigger than theirs, then use ours
+ //otherwise use theirs
+ if ( ours.getValue(Type.LONG) > theirs.getValue(Type.LONG) ){
+ merged.setValue(ours.getValue(Type.LONG));
+ } else {
+ merged.setValue(theirs.getValue(Type.LONG));
+ }
+ parent.setProperty(merged.getPropertyState());
+ LOG.debug("Resolved conflict for property {} our value: {}, merged value: {}", REP_EXPIRATION, ours.getValue(Type.LONG), merged.getValue(0));
Review Comment:
is there a reason you only log our value and not their value as well?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
+ * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li>
+ * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li>
+ * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li>
+ * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li>
+ * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li>
+ * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li>
+ * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li>
+ * </ul>
+ */
+
+public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler {
+
+ private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class);
+
+ protected static final String REP_EXPIRATION = "rep:expiration";
+ protected static final String REP_CACHE = "rep:cache";
+
+ /**
+ * Create a new {@code ConflictHandler} which always returns
+ * {@code resolution}.
+ *
+ * @param resolution the resolution to return from all methods of this
+ * {@code ConflictHandler} instance.
+ */
+ public PrincipalCacheConflictHandler(Resolution resolution) {
+ super(resolution);
+ }
+ public PrincipalCacheConflictHandler() {
+ this(Resolution.IGNORED);
+ }
+
+ private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs,
Review Comment:
it would be good if this method would follow common coding style as we use it elsewhere in oak.
so, instead of
`if ( condition1 && condition2 ){}`
it would be
`if (condition1 && condition2) {}`
either run reformat code or if needed adjust the formatting settings of your ide. i believe we use standard java coding style.
if i reformat it, it looks as follows:
```
private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs,
PropertyState base) {
if (REP_EXPIRATION.equals(ours.getName()) && REP_EXPIRATION.equals(theirs.getName())) {
PropertyBuilder<Long> merged = PropertyBuilder.scalar(Type.LONG);
merged.setName(REP_EXPIRATION);
//if base is bigger than ours and theirs, then use base. This should never happens
if (base != null &&
base.getValue(Type.LONG) > ours.getValue(Type.LONG) &&
base.getValue(Type.LONG) > theirs.getValue(Type.LONG)) {
merged.setValue(base.getValue(Type.LONG));
return Resolution.MERGED;
}
//if ours is bigger than theirs, then use ours
//otherwise use theirs
if (ours.getValue(Type.LONG) > theirs.getValue(Type.LONG)) {
merged.setValue(ours.getValue(Type.LONG));
} else {
merged.setValue(theirs.getValue(Type.LONG));
}
parent.setProperty(merged.getPropertyState());
LOG.debug("Resolved conflict for property {} our value: {}, merged value: {}", REP_EXPIRATION, ours.getValue(Type.LONG), merged.getValue(0));
return Resolution.MERGED;
}
return Resolution.IGNORED;
}
```
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
+ * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li>
+ * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li>
+ * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li>
+ * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li>
+ * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li>
+ * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li>
+ * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li>
+ * </ul>
+ */
+
+public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler {
+
+ private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class);
+
+ protected static final String REP_EXPIRATION = "rep:expiration";
+ protected static final String REP_CACHE = "rep:cache";
+
+ /**
+ * Create a new {@code ConflictHandler} which always returns
+ * {@code resolution}.
+ *
+ * @param resolution the resolution to return from all methods of this
+ * {@code ConflictHandler} instance.
+ */
+ public PrincipalCacheConflictHandler(Resolution resolution) {
+ super(resolution);
+ }
+ public PrincipalCacheConflictHandler() {
+ this(Resolution.IGNORED);
+ }
+
+ private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs,
+ PropertyState base) {
+ if ( REP_EXPIRATION.equals(ours.getName()) && REP_EXPIRATION.equals(theirs.getName()) ){
+
+ PropertyBuilder<Long> merged = PropertyBuilder.scalar(Type.LONG);
+ merged.setName(REP_EXPIRATION);
+
+ //if base is bigger than ours and theirs, then use base. This should never happens
+ if ( base != null &&
+ base.getValue(Type.LONG) > ours.getValue(Type.LONG) &&
+ base.getValue(Type.LONG) > theirs.getValue(Type.LONG) ){
+ merged.setValue(base.getValue(Type.LONG));
+ return Resolution.MERGED;
+ }
+
+ //if ours is bigger than theirs, then use ours
+ //otherwise use theirs
+ if ( ours.getValue(Type.LONG) > theirs.getValue(Type.LONG) ){
+ merged.setValue(ours.getValue(Type.LONG));
+ } else {
+ merged.setValue(theirs.getValue(Type.LONG));
+ }
+ parent.setProperty(merged.getPropertyState());
+ LOG.debug("Resolved conflict for property {} our value: {}, merged value: {}", REP_EXPIRATION, ours.getValue(Type.LONG), merged.getValue(0));
+ return Resolution.MERGED;
+ }
+ return Resolution.IGNORED;
+
+ }
+
+ @Override
+ public Resolution changeChangedProperty(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs,
+ @NotNull PropertyState base) {
+
+ return resolveRepExpirationConflict(parent, ours, theirs, base);
Review Comment:
see initial comment: apart from the expiration also the principal-names could conflict... i don't recall if that was originally part of the reported conflicts, but we may want to consider it.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -287,7 +287,16 @@ private Set<Principal> getGroupMembership(@NotNull Tree authorizableTree) {
// remember the regular groups in case caching is enabled
if (doCache) {
- cacheGroups(authorizableTree, groupPrincipals);
+ PrincipalCommitterThread commitThread = UserPrincipalProviderCommitterProvider.getInstance().cacheGroups(authorizableTree, groupPrincipals, expiration, root);
+ if (commitThread != null) {
+ //We need to wait the thread to finish, otherwise the session will be closed before the commit is done
+ try {
+ commitThread.join();
+ } catch (InterruptedException e) {
+ log.error("Unexpected Error while waiting for commit thread to finish", e);
Review Comment:
IMHO this error messages should be more precise explaining what the nature of this exception is.... i.e. explaining that it's about writing the principal cache.
also, i would recommend to lower the log level to warning. after all there is nothing the oak-consumer did wrong here or could have done differently. an error imo should be actionable, which this problem is not, right?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
+ * during parallel updates.
+ *<p>
+ * The conflict handler deals with the following conflicts:
+ * <ul>
+ * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li>
+ * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li>
+ * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li>
+ * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li>
+ * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li>
+ * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li>
+ * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li>
+ * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li>
+ * </ul>
+ */
+
+public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler {
+
+ private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class);
+
+ protected static final String REP_EXPIRATION = "rep:expiration";
+ protected static final String REP_CACHE = "rep:cache";
+
+ /**
+ * Create a new {@code ConflictHandler} which always returns
+ * {@code resolution}.
+ *
+ * @param resolution the resolution to return from all methods of this
+ * {@code ConflictHandler} instance.
+ */
+ public PrincipalCacheConflictHandler(Resolution resolution) {
+ super(resolution);
+ }
+ public PrincipalCacheConflictHandler() {
+ this(Resolution.IGNORED);
+ }
+
+ private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs,
+ PropertyState base) {
+ if ( REP_EXPIRATION.equals(ours.getName()) && REP_EXPIRATION.equals(theirs.getName()) ){
+
+ PropertyBuilder<Long> merged = PropertyBuilder.scalar(Type.LONG);
+ merged.setName(REP_EXPIRATION);
+
+ //if base is bigger than ours and theirs, then use base. This should never happens
Review Comment:
if it should never happen, should we log something as this is an anomality?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
Review Comment:
i wonder if it might be needed to also cover conflicts with the rep:principalsNames property?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+public class UserPrincipalProviderCommitterProvider {
+
+ static UserPrincipalProviderCommitterProvider instance = null;
Review Comment:
this should be private, no?
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/AbstractSecurityTest.java:
##########
@@ -89,8 +89,8 @@ public abstract class AbstractSecurityTest {
protected Root root;
protected QueryEngineSettings querySettings;
- private final RootProvider rootProvider = new RootProviderService();
- private final TreeProvider treeProvider = new TreeProviderService();
+ protected final RootProvider rootProvider = new RootProviderService();
Review Comment:
please use getRootProvider() in the conflict-handler test instead of altering the access modifier
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+public class UserPrincipalProviderCommitterProvider {
+
+ static UserPrincipalProviderCommitterProvider instance = null;
+ static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>();
Review Comment:
nitpicking: should be private static final as it only be used inside the UserPrincipalProviderCommitterProvider
on a more serious note: since the map is a singleton i believe it should deal with concurrent modifications as there might be concurrent logins for different users.
and another thing:
i am not super happy that the map is passed to the thread and the cleanup is done there. at a first glance i thought this was a bug because i didn't see the remove of the entry. so, if possible i would keep the cleanup in the provider.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+public class UserPrincipalProviderCommitterProvider {
Review Comment:
i believe this class doesn't need to be public but can be package protected.
naming: the name of the class is a bit hard to process with the duplicate 'Provider' in it.
what about something that more precisely describes its function? CacheThreadProvider? this would make it end up being 'close' to `CacheConstants` and `CacheValidatorProvider` in whose proximity it belongs.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.jetbrains.annotations.NotNull;
+
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+public class UserPrincipalProviderCommitterProvider {
+
+ static UserPrincipalProviderCommitterProvider instance = null;
+ static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>();
+
+ public static UserPrincipalProviderCommitterProvider getInstance() {
+ if (instance == null) {
+ instance = new UserPrincipalProviderCommitterProvider();
+ }
+ return instance;
+ }
+
+ public synchronized PrincipalCommitterThread cacheGroups(@NotNull Tree authorizableNode, @NotNull Set<Principal> groupPrincipals, long expiration, Root root) {
+ String authorizableNodePath = authorizableNode.getPath();
+ if (committerThreadMap.containsKey(authorizableNodePath)) {
+ // One thread is already committing. return null to inform the caller that doesn't have to wait for the commit to finish
+ return null;
+ } else {
+ PrincipalCommitterThread committerThread = new PrincipalCommitterThread(authorizableNode, groupPrincipals, expiration, root, committerThreadMap);
+ committerThreadMap.put(authorizableNodePath, committerThread);
+ committerThread.start();
+ return committerThread;
+ }
+ }
+
+ public HashMap<String, PrincipalCommitterThread> getCommitterThreadMap() {
Review Comment:
as far as i can see this method is only used for test verification. i would recommend to drop it and use reflection to assert the state of the map in test-cases..... that leaves no ambiguity and doesn't risk leaking the map (and have it manipulated) by the next dev touching this.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCommitterThread.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.util.Text;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.jcr.AccessDeniedException;
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.MEMBERSHIP_THRESHOLD;
Review Comment:
the constant doesn't need to be imported but could simply be moved here as it is not used elsewhere
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java:
##########
@@ -287,7 +287,16 @@ private Set<Principal> getGroupMembership(@NotNull Tree authorizableTree) {
// remember the regular groups in case caching is enabled
if (doCache) {
- cacheGroups(authorizableTree, groupPrincipals);
+ PrincipalCommitterThread commitThread = UserPrincipalProviderCommitterProvider.getInstance().cacheGroups(authorizableTree, groupPrincipals, expiration, root);
Review Comment:
the commitThread could just be a Thread object. it the userprincipalprovider can be ignorant about the implementation, no?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCommitterThread.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.util.Text;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.jcr.AccessDeniedException;
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.MEMBERSHIP_THRESHOLD;
+
+public class PrincipalCommitterThread extends Thread {
+ Tree authorizableNode;
+ Set<Principal> groupPrincipals;
+ HashMap committerThreadMap;
+ long expiration;
+ Root root;
+
+ private static final Logger log = LoggerFactory.getLogger(PrincipalCommitterThread.class);
+
+ public PrincipalCommitterThread(Tree authorizableNode, Set<Principal> groupPrincipals, long expiration, Root root, HashMap committerThreadMap) {
Review Comment:
- constructor should not be public
- params should be annotation with `@NotNull` (except the expiration)
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCommitterThread.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.util.Text;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.jcr.AccessDeniedException;
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.MEMBERSHIP_THRESHOLD;
+
+public class PrincipalCommitterThread extends Thread {
Review Comment:
i would recommend to move this class as an inner class of the UserPrincipalProviderCommitterProvider. nobody outside needs to know it, right?
that might also solve the issue with passing around the map and the removal of entries on that map that i complained about in UserPrincipalProviderCommitterProvider.
at the very least i must not be a public class.
naming: it's not precisely about committing principals but about the cache, right? so i would reconsider the naming (but that would be sort of irrelevant if it were a private inner class)
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCommitterThread.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.guava.common.base.Joiner;
+import org.apache.jackrabbit.guava.common.collect.Iterables;
+import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.LongUtils;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.util.Text;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.jcr.AccessDeniedException;
+import java.security.Principal;
+import java.util.HashMap;
+import java.util.Set;
+
+import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.MEMBERSHIP_THRESHOLD;
+
+public class PrincipalCommitterThread extends Thread {
+ Tree authorizableNode;
+ Set<Principal> groupPrincipals;
+ HashMap committerThreadMap;
+ long expiration;
+ Root root;
Review Comment:
all instance fields of this class should be `private final`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#issuecomment-1782420442
> I would take a slightly different approach and not start new threads to perform the cache update.
>
> See draft in https://github.com/mreutegg/jackrabbit-oak/tree/OAK-10451
>
> It also introduces a maxStale configuration, though not yet added to UserConfigurationImpl, to set an upper bound for how long the cache entry can be served when it is not up to date. As mentioned, this is just a draft to illustrate the approach and unit tests are missing.
@mreutegg I agree with that your approach. I added Unit tests to your classes and pushed them in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "nscendoni (via GitHub)" <gi...@apache.org>.
nscendoni commented on code in PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1349988935
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.jackrabbit.oak.security.user;
+
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
+import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property
Review Comment:
I'm not sure. They are much more unlikely...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10451 UserPrincipalProvider may cause many conflicts when under load [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on PR #1128:
URL: https://github.com/apache/jackrabbit-oak/pull/1128#issuecomment-2149051571
@mreutegg , since you were the author of the initial proposal, I would like to get another review from your side to see if the refactoring I made last week, still meets your approach.
most importantly: I found the logic and when to remove entries from the updates-map not easy to understand and therefore refactored the code such that the updates-map will always get the entry removed upon completion (successful or unsuccessful). in other words: if there is no entry for a given path, it is added notifying other threads about a current update and in the finally clause it is always removed. at the same time i removed the logic of keeping the previous exp-time (or 0) as value, which i didn't find intuitive to understand even after several reviews and instead used the thread-id as value.
wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org