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

Resolve ImmutableOpenMap issue from core refactor #2715

Merged
merged 7 commits into from
Apr 25, 2023
Merged

Resolve ImmutableOpenMap issue from core refactor #2715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3575d98
remove immutable open map use
stephen-crawford Apr 21, 2023
cbfa154
Remove alias dependency
stephen-crawford Apr 24, 2023
d40f98b
Swap over AliasExistsMatcher
stephen-crawford Apr 24, 2023
cf6788d
Fix indexMappingIsEqual
stephen-crawford Apr 24, 2023
47f08b9
Merge branch 'opensearch-project:main' into valuesItFix
stephen-crawford Apr 24, 2023
2b6774b
Try to fix privEval iterator
stephen-crawford Apr 24, 2023
876871e
Update iterator
stephen-crawford Apr 24, 2023
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
15 changes: 11 additions & 4 deletions src/integrationTest/java/org/opensearch/test/framework/matcher/AliasExistsMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,22 @@
package org.opensearch.test.framework.matcher;

import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;

import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
import org.hamcrest.Description;
import org.hamcrest.TypeSafeDiagnosingMatcher;

import org.opensearch.action.admin.indices.alias.get.GetAliasesRequest;
import org.opensearch.action.admin.indices.alias.get.GetAliasesResponse;
import org.opensearch.client.Client;
import org.opensearch.cluster.metadata.AliasMetadata;
import org.opensearch.common.collect.ImmutableOpenMap;

import static java.util.Objects.requireNonNull;
import static java.util.Spliterator.IMMUTABLE;
Expand All @@ -41,8 +43,13 @@ public AliasExistsMatcher(String aliasName) {
protected boolean matchesSafely(Client client, Description mismatchDescription) {
try {
GetAliasesResponse response = client.admin().indices().getAliases(new GetAliasesRequest(aliasName)).get();
ImmutableOpenMap<String, List<AliasMetadata>> aliases = response.getAliases();
Set<String> actualAliasNames = StreamSupport.stream(spliteratorUnknownSize(aliases.valuesIt(), IMMUTABLE), false)

final Map<String, List<AliasMetadata>> aliases = new HashMap<>();
for (ObjectObjectCursor<String, List<AliasMetadata>> cursor : response.getAliases()) {
aliases.put(cursor.key, cursor.value);
}

Set<String> actualAliasNames = StreamSupport.stream(spliteratorUnknownSize(aliases.values().iterator(), IMMUTABLE), false)
.flatMap(Collection::stream)
.map(AliasMetadata::getAlias)
.collect(Collectors.toSet());
Expand All @@ -53,7 +60,7 @@ protected boolean matchesSafely(Client client, Description mismatchDescription)
}
return true;
} catch (InterruptedException | ExecutionException e) {
mismatchDescription.appendText("Error occured during checking if cluster contains alias ")
mismatchDescription.appendText("Error occurred during checking if cluster contains alias ")
.appendValue(e);
return false;
}
Expand Down
19 changes: 15 additions & 4 deletions ...st/java/org/opensearch/test/framework/matcher/ClusterContainTemplateWithAliasMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,21 @@
*/
package org.opensearch.test.framework.matcher;

import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;

import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
import org.hamcrest.Description;
import org.hamcrest.TypeSafeDiagnosingMatcher;

import org.opensearch.action.admin.indices.template.get.GetIndexTemplatesRequest;
import org.opensearch.action.admin.indices.template.get.GetIndexTemplatesResponse;
import org.opensearch.client.Client;
import org.opensearch.cluster.metadata.AliasMetadata;
import org.opensearch.common.collect.ImmutableOpenMap;

import static java.util.Objects.requireNonNull;

Expand Down Expand Up @@ -55,15 +57,24 @@ protected boolean matchesSafely(Client client, Description mismatchDescription)
private Set<String> getAliases(GetIndexTemplatesResponse response) {
return response.getIndexTemplates()
.stream()
.map(metadata -> metadata.getAliases())
.map(metadata -> {
Map<String, AliasMetadata> aliases = new HashMap<>();
for (ObjectObjectCursor<String, AliasMetadata> cursor : metadata.getAliases()) {
aliases.put(cursor.key, cursor.value);
}
return aliases;
})
.flatMap(aliasMap -> aliasNames(aliasMap))
.collect(Collectors.toSet());
}

private Stream<String> aliasNames(ImmutableOpenMap<String, AliasMetadata> aliasMap) {
return StreamSupport.stream(aliasMap.keys().spliterator(), false).map(objectCursor -> objectCursor.value);
private Stream<String> aliasNames(Map<String, AliasMetadata> aliasMap) {
Iterable<Map.Entry<String, AliasMetadata>> iterable = () -> aliasMap.entrySet().iterator();
return StreamSupport.stream(iterable.spliterator(), false)
.map(entry -> entry.getValue().getAlias());
}


@Override
public void describeTo(Description description) {
description.appendText("template ").appendValue(templateName).appendText(" exists and ");
Expand Down
13 changes: 10 additions & 3 deletions ...java/org/opensearch/test/framework/matcher/GetSettingsResponseContainsIndicesMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@
*/
package org.opensearch.test.framework.matcher;

import java.util.HashMap;
import java.util.Map;

import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
import org.hamcrest.Description;
import org.hamcrest.TypeSafeDiagnosingMatcher;

import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse;
import org.opensearch.common.collect.ImmutableOpenMap;
import org.opensearch.common.settings.Settings;

import static java.util.Objects.isNull;
Expand All @@ -31,12 +34,16 @@ class GetSettingsResponseContainsIndicesMatcher extends TypeSafeDiagnosingMatche

@Override
protected boolean matchesSafely(GetSettingsResponse response, Description mismatchDescription) {
ImmutableOpenMap<String, Settings> indexToSettings = response.getIndexToSettings();

final Map<String, Settings> indexToSettings = new HashMap<>();
for (ObjectObjectCursor<String, Settings> cursor : response.getIndexToSettings()) {
indexToSettings.put(cursor.key, cursor.value);
}
Comment on lines +38 to +41
Copy link
Member

Choose a reason for hiding this comment

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

Can this be simplified to:

Suggested change
final Map<String, Settings> indexToSettings = new HashMap<>();
for (ObjectObjectCursor<String, Settings> cursor : response.getIndexToSettings()) {
indexToSettings.put(cursor.key, cursor.value);
}
final Set<String> indexKeys = new HashSet<>();
for (ObjectObjectCursor<String, Settings> cursor : response.getIndexToSettings()) {
indexKeys.add(cursor.key);
}

Copy link
Member

Choose a reason for hiding this comment

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

Looks like either would work, but since we are not using the values in the map and only getting a keySet it would take less memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry Craig, I didn't see this before the PR got merged. I had left it as a map because that is what the original implementation was. I agree that a Set should work fine here as well. If you wanted to swap it you could open up a quick PR to change it in main.

for (String index : expectedIndices) {
if (!indexToSettings.containsKey(index)) {
mismatchDescription
.appendText("Response contains settings of indices: ")
.appendValue(indexToSettings.keys());
.appendValue(indexToSettings.keySet());
return false;
}
}
Expand Down
5 changes: 1 addition & 4 deletions ...egrationTest/java/org/opensearch/test/framework/matcher/IndexMappingIsEqualToMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
import org.opensearch.action.admin.indices.mapping.get.GetMappingsRequest;
import org.opensearch.action.admin.indices.mapping.get.GetMappingsResponse;
import org.opensearch.client.Client;
import org.opensearch.cluster.metadata.MappingMetadata;
import org.opensearch.common.collect.ImmutableOpenMap;
import org.opensearch.index.IndexNotFoundException;
import org.opensearch.test.framework.cluster.LocalCluster;

Expand All @@ -44,8 +42,7 @@ protected boolean matchesSafely(LocalCluster cluster, Description mismatchDescri
GetMappingsResponse response = client.admin().indices()
.getMappings(new GetMappingsRequest().indices(expectedIndexName)).actionGet();

ImmutableOpenMap<String, MappingMetadata> actualMappings = response.mappings();
Map<String, Object> actualIndexMapping = actualMappings.get(expectedIndexName).getSourceAsMap();
Map<String, Object> actualIndexMapping = response.getMappings().get(expectedIndexName).sourceAsMap();

if (!expectedMapping.equals(actualIndexMapping)) {
mismatchDescription.appendText("Actual mapping ").appendValue(actualIndexMapping).appendText(" does not match expected");
Expand Down
5 changes: 3 additions & 2 deletions ...ntegrationTest/java/org/opensearch/test/framework/matcher/IndexStateIsEqualToMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@
*/
package org.opensearch.test.framework.matcher;

import java.util.Map;

import org.hamcrest.Description;
import org.hamcrest.TypeSafeDiagnosingMatcher;

import org.opensearch.action.admin.cluster.state.ClusterStateRequest;
import org.opensearch.action.admin.cluster.state.ClusterStateResponse;
import org.opensearch.client.Client;
import org.opensearch.cluster.metadata.IndexMetadata;
import org.opensearch.common.collect.ImmutableOpenMap;
import org.opensearch.test.framework.cluster.LocalCluster;

import static java.util.Objects.requireNonNull;
Expand All @@ -37,7 +38,7 @@ protected boolean matchesSafely(LocalCluster cluster, Description mismatchDescri
ClusterStateRequest clusterStateRequest = new ClusterStateRequest().indices(expectedIndexName);
ClusterStateResponse clusterStateResponse = client.admin().cluster().state(clusterStateRequest).actionGet();

ImmutableOpenMap<String, IndexMetadata> indicesMetadata = clusterStateResponse.getState().getMetadata().indices();
Map<String, IndexMetadata> indicesMetadata = clusterStateResponse.getState().getMetadata().indices();
if (!indicesMetadata.containsKey(expectedIndexName)) {
mismatchDescription.appendValue("Index does not exist");
}
Expand Down
15 changes: 9 additions & 6 deletions src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
Expand All @@ -37,6 +38,7 @@
import java.util.StringJoiner;
import java.util.regex.Pattern;

import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import org.apache.logging.log4j.LogManager;
Expand Down Expand Up @@ -78,7 +80,6 @@
import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.Strings;
import org.opensearch.common.collect.ImmutableOpenMap;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.transport.TransportAddress;
import org.opensearch.common.util.concurrent.ThreadContext;
Expand Down Expand Up @@ -378,8 +379,6 @@ public PrivilegesEvaluatorResponse evaluate(final User user, String action0, fin
presponse.allowed = true;
return presponse;
}


}
}

Expand Down Expand Up @@ -632,6 +631,7 @@ public static boolean isClusterPerm(String action0) {
) ;
}

@SuppressWarnings("unchecked")
private boolean checkFilteredAliases(Resolved requestedResolved, String action, boolean isDebugEnabled) {
final String faMode = dcm.getFilteredAliasMode();// getConfigSettings().dynamic.filtered_alias_mode;

Expand All @@ -649,7 +649,7 @@ private boolean checkFilteredAliases(Resolved requestedResolved, String action,
indexMetaDataCollection = new Iterable<IndexMetadata>() {
@Override
public Iterator<IndexMetadata> iterator() {
return clusterService.state().getMetadata().getIndices().valuesIt();
return clusterService.state().getMetadata().getIndices().values().iterator();
}
};
} else {
Expand All @@ -674,14 +674,17 @@ public Iterator<IndexMetadata> iterator() {

final List<AliasMetadata> filteredAliases = new ArrayList<AliasMetadata>();

final ImmutableOpenMap<String, AliasMetadata> aliases = indexMetaData.getAliases();
final Map<String, AliasMetadata> aliases = new HashMap<>();
Copy link
Member

@andrross andrross Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@scrawfor99 Why did you make this change? Doesn't indexMetaData.getAliases() still return an ImmutableOpenMap? Seems like you should avoid making a copy of the map in any case.

Copy link
Contributor Author

@stephen-crawford stephen-crawford Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @andrross, I made this swap because we could not use keysIt anymore. We needed aliases to be a normal map to then grab and iterate over the keyset. Is there an issue with this implementation?

Copy link
Member

Choose a reason for hiding this comment

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

@nknize What's happening here? Pretty sure making a copy of the the map is the exact opposite of your intention with this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the issue that arose when the change was made to core: #2714. I just made changes to get things Green. Sorry if this was not the intended implementation...

Copy link
Member

Choose a reason for hiding this comment

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

Sorry @scrawfor99! Definitely not trying to blame you here! I don't understand why keysIt doesn't work anymore, and I don't think the intent with @nknize's change is to force downstream dependencies to copy data structures when doing a simple iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @andrross, no need to apologize :). I just want to make sure I did the correct thing. I did not look that closely into the overall reasoning for the change in core. I just checked to find the overall cause of the issue we were facing and then took the fastest route to unblocking. If you or Nick, have an alternative solution that is the preferred method now just let me know and I can make a swap. In the linked issue, I showed an example of the errors we were getting which was basically Java complaining about not being able to find the ImmutableOpenMap method definitions.

Copy link

Choose a reason for hiding this comment

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

No copies unless you need to make changes to the map (even then the core should return an unmodifiableMap so you would only be able to change a clone).

If you just need an iterator over the keys then use .keySet().iterator()

Copy link
Member

Choose a reason for hiding this comment

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

@scrawfor99 The code in question changed to be a plain map. I think you should be able to simplify this and get rid of the copy.

for (ObjectObjectCursor<String, AliasMetadata> cursor : indexMetaData.getAliases()) {
aliases.put(cursor.key, cursor.value);
}

if(aliases != null && aliases.size() > 0) {
if (isDebugEnabled) {
log.debug("Aliases for {}: {}", indexMetaData.getIndex().getName(), aliases);
}

final Iterator<String> it = aliases.keysIt();
final Iterator<String> it = aliases.keySet().iterator();
while(it.hasNext()) {
final String alias = it.next();
final AliasMetadata aliasMetadata = aliases.get(alias);
Expand Down