Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove commons-collections 3.2.2 #2924

Merged
merged 1 commit into from
Jul 7, 2023
Merged

Remove commons-collections 3.2.2 #2924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ dependencies {
implementation "org.opensaml:opensaml-storage-api:${open_saml_version}"

implementation "com.nulab-inc:zxcvbn:1.7.0"
implementation 'commons-collections:commons-collections:3.2.2'

implementation 'com.jayway.jsonpath:json-path:2.4.0'
implementation 'net.minidev:json-smart:2.4.10'
runtimeOnly 'io.jsonwebtoken:jjwt-impl:0.10.8'
Expand Down
27 changes: 13 additions & 14 deletions src/integrationTest/java/org/opensearch/test/framework/log/LogCapturingAppender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,8 @@
*/
package org.opensearch.test.framework.log;

import java.io.Serializable;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;

import org.apache.commons.collections.Buffer;
import org.apache.commons.collections.BufferUtils;
import org.apache.commons.collections.buffer.CircularFifoBuffer;
import com.google.common.collect.EvictingQueue;
import com.google.common.collect.Queues;
import org.apache.logging.log4j.core.Appender;
import org.apache.logging.log4j.core.Core;
import org.apache.logging.log4j.core.Filter;
Expand All @@ -32,6 +22,15 @@
import org.apache.logging.log4j.core.config.plugins.PluginAttribute;
import org.apache.logging.log4j.core.config.plugins.PluginFactory;

import java.io.Serializable;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Queue;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;

import static org.opensearch.test.framework.log.LogCapturingAppender.PLUGIN_NAME;

/**
Expand All @@ -56,12 +55,12 @@ public class LogCapturingAppender extends AbstractAppender {
/**
* Buffer for captured log messages
*/
private static final Buffer messages = BufferUtils.synchronizedBuffer(new CircularFifoBuffer(MAX_SIZE));
private static final Queue<LogMessage> messages = Queues.synchronizedQueue(EvictingQueue.create(MAX_SIZE));

/**
* Log messages are stored in buffer {@link #messages} only for classes which are added to the {@link #activeLoggers} set.
*/
private static final Set<String> activeLoggers = Collections.synchronizedSet(new HashSet<>());
private static final Set<String> activeLoggers = ConcurrentHashMap.newKeySet();

protected LogCapturingAppender(
String name,
Expand Down
60 changes: 52 additions & 8 deletions src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.regex.PatternSyntaxException;
import java.util.stream.Collectors;

import com.google.common.collect.ImmutableSet;
import org.apache.commons.collections.keyvalue.MultiKey;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.greenrobot.eventbus.Subscribe;
Expand Down Expand Up @@ -112,7 +112,7 @@ public IndexResolverReplacer(IndexNameExpressionResolver resolver, ClusterServic
this.clusterInfoHolder = clusterInfoHolder;
}

private static final boolean isAllWithNoRemote(final String... requestedPatterns) {
private static boolean isAllWithNoRemote(final String... requestedPatterns) {

final List<String> patterns = requestedPatterns == null ? null : Arrays.asList(requestedPatterns);

Expand All @@ -131,11 +131,11 @@ private static final boolean isAllWithNoRemote(final String... requestedPatterns
return false;
}

private static final boolean isLocalAll(String... requestedPatterns) {
private static boolean isLocalAll(String... requestedPatterns) {
return isLocalAll(requestedPatterns == null ? null : Arrays.asList(requestedPatterns));
}

private static final boolean isLocalAll(Collection<String> patterns) {
private static boolean isLocalAll(Collection<String> patterns) {
if (IndexNameExpressionResolver.isAllIndices(patterns)) {
return true;
}
Expand All @@ -158,9 +158,49 @@ private class ResolvedIndicesProvider implements IndicesProvider {
private final ImmutableSet.Builder<String> remoteIndices;
// set of previously resolved index requests to avoid resolving
// the same index more than once while processing bulk requests
private final Set<MultiKey> alreadyResolved;
private final Set<AlreadyResolvedKey> alreadyResolved;
Copy link
Collaborator Author

@willyborankin willyborankin Jul 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. But using unbounded collection could lead to a memory leak. So a question to those who knows this part better than me. Is it possible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to prevent the chance of a memory leak entirely, I believe we would need to cap the size of the set. The problem is determining an appropriate cap for the size. I believe the set should be cleared with each request so it should not persist for too long, so the issue would only be that we ran into a memory issue in a single bulk request. For determining the size to cap it at, we could look at the expected size of each AlreadyResolvedKey object and then determine how much memory we want to allow the task to allocate. Obviously, we will reach issues at certain extremes unless we can start to calculate based off of node resources.

private final String name;

private final class AlreadyResolvedKey {

private final IndicesOptions indicesOptions;

private final boolean enableCrossClusterResolution;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I am missing something, but how is this field being used? Is this part of the bulk request logic in some way? I am not particularly familiar with this--are we using this field to differentiate between two instances of the same index operation but with different CCR permissions?

Copy link
Collaborator Author

@willyborankin willyborankin Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enableCrossClusterResolution determines here:

security/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java

Line 375 in 1089bd6

final boolean enableCrossClusterResolution = localRequest instanceof FieldCapabilitiesRequest
.
I played with it a bit. If I remove it all tests pass except PIT tests. TBH did not investigate why.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries.


private final String[] original;

private AlreadyResolvedKey(final IndicesOptions indicesOptions, final boolean enableCrossClusterResolution) {
this(indicesOptions, enableCrossClusterResolution, null);
}

private AlreadyResolvedKey(
final IndicesOptions indicesOptions,
final boolean enableCrossClusterResolution,
final String[] original
) {
this.indicesOptions = indicesOptions;
this.enableCrossClusterResolution = enableCrossClusterResolution;
this.original = original;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
AlreadyResolvedKey that = (AlreadyResolvedKey) o;
return enableCrossClusterResolution == that.enableCrossClusterResolution
&& Objects.equals(indicesOptions, that.indicesOptions)
&& Arrays.equals(original, that.original);
}

@Override
public int hashCode() {
int result = Objects.hash(indicesOptions, enableCrossClusterResolution);
result = 31 * result + Arrays.hashCode(original);
return result;
}
}

ResolvedIndicesProvider(Object request) {
aliases = ImmutableSet.builder();
allIndices = ImmutableSet.builder();
Expand Down Expand Up @@ -336,9 +376,13 @@ public String[] provide(String[] original, Object localRequest, boolean supports
|| localRequest instanceof SearchRequest
|| localRequest instanceof ResolveIndexAction.Request;
// skip the whole thing if we have seen this exact resolveIndexPatterns request
if (alreadyResolved.add(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks fine to me. I was also looking at where alreadyResolved was used and it looks like this line is primarily why a MultiKey was chosen. IMO the logic utilizing MultiKey was hard to follow and this is clearer.

new MultiKey(indicesOptions, enableCrossClusterResolution, (original != null) ? new MultiKey(original, false) : null)
)) {
final AlreadyResolvedKey alreadyResolvedKey;
if (original != null) {
alreadyResolvedKey = new AlreadyResolvedKey(indicesOptions, enableCrossClusterResolution, original);
} else {
alreadyResolvedKey = new AlreadyResolvedKey(indicesOptions, enableCrossClusterResolution);
}
if (alreadyResolved.add(alreadyResolvedKey)) {
resolveIndexPatterns(localRequest.getClass().getSimpleName(), indicesOptions, enableCrossClusterResolution, original);
}
return IndicesProvider.NOOP;
Expand Down