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-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
