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

Jump to bottom

ISPN-14568 Cache Alias #11726

Merged
merged 2 commits into from May 3, 2024
Merged

ISPN-14568 Cache Alias #11726

merged 2 commits into from May 3, 2024

Conversation

tristantarrant
Copy link
Member

@tristantarrant tristantarrant mentioned this pull request Jan 30, 2024
Merged
@tristantarrant tristantarrant added this to the 15.0.0.Dev09 milestone Feb 2, 2024
ryanemerson
ryanemerson requested changes Feb 15, 2024
View reviewed changes
core/src/main/java/org/infinispan/configuration/cache/Configuration.java Show resolved Hide resolved
core/src/main/java/org/infinispan/configuration/ConfigurationManager.java Outdated Show resolved Hide resolved
core/src/main/java/org/infinispan/configuration/ConfigurationManager.java Outdated Show resolved Hide resolved
core/src/main/java/org/infinispan/util/logging/Log.java Outdated Show resolved Hide resolved
core/src/test/java/org/infinispan/api/CacheAPITest.java Show resolved Hide resolved
core/src/test/java/org/infinispan/api/CacheAliasTest.java Outdated Show resolved Hide resolved
core/src/test/java/org/infinispan/commands/module/TestGlobalConfigurationBuilder.java Show resolved Hide resolved
server/runtime/src/main/server/server/conf/infinispan-dev-mode.xml Outdated Show resolved Hide resolved
server/rest/src/main/java/org/infinispan/rest/resources/ServerResource.java
import java.util.Map;
import java.util.LinkedHashMap;
import java.util.Set;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated change, can be reverted.

server/resp/src/main/java/org/infinispan/server/resp/commands/connection/SELECT.java
Consumers.OK_BICONSUMER.accept(null, handler.allocator());
} else {
ByteBufferUtils.stringToByteBuf("-ERR DB index is out of range\r\n", handler.allocator());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

My Redis/RESP knowledge is lacking here. I understand that we're adding an alias of 0 by default, but what must be configured in order for an Infinispan user to do SELECT 1? This should be documented.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a chapter to the redis connector docs

wburns
wburns reviewed Feb 15, 2024
View reviewed changes
Copy link
Member

@wburns wburns left a comment

Choose a reason for hiding this comment

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

Looking good, just some minor concerns

client/rest-client/src/main/java/org/infinispan/client/rest/impl/jdk/RestCacheClientJDK.java Outdated Show resolved Hide resolved
...s/all/src/main/java/org/infinispan/commons/configuration/attributes/AttributeDefinition.java
@@ -229,6 +234,11 @@ public Builder<T> initializer(AttributeInitializer<? extends T> initializer) {
return this;
}

public Builder<T> matcher(AttributeMatcher<T> matcher) {
Copy link
Member

Choose a reason for hiding this comment

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

I see the only use is to always return true for the matcher. Do you expect to see other implementations?

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment the need is only theoretical.

core/src/main/java/org/infinispan/configuration/ConfigurationManager.java
@@ -119,4 +130,55 @@ public ConfigurationBuilderHolder toBuilderHolder() {
}
return holder;
}

private void addAliases(String cacheName, Collection<String> aliases) {
synchronized (this.aliases) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like there is an issue here that if you have an alias setup and then you define a new configuration that "overwrites" the alias.

I think we need to double check that the cacheName is not present in the aliases map as a value.

core/src/main/resources/schema/infinispan-config-15.0.xsd Outdated Show resolved Hide resolved
server/resp/src/main/java/org/infinispan/server/resp/CacheRespRequestHandler.java Outdated Show resolved Hide resolved
server/resp/src/main/java/org/infinispan/server/resp/RespServer.java Outdated Show resolved Hide resolved
server/rest/src/main/java/org/infinispan/rest/resources/CacheResourceV2.java
@@ -942,7 +942,7 @@ private CompletionStage<RestResponse> getCacheConfigMutableAttribute(RestRequest
private CompletionStage<RestResponse> setCacheConfigMutableAttribute(RestRequest request) {
NettyRestResponse.Builder responseBuilder = invocationHelper.newResponse(request);
String attributeName = request.getParameter("attribute-name");
String attributeValue = request.getParameter("attribute-value");
String attributeValue = String.join(" ", request.parameters().get("attribute-value"));
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a pretty big change for other things that take parameters as well outside of this...

Copy link
Member Author

Choose a reason for hiding this comment

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

attribute.fromString() already supports splitting space-separated multi-value attributes (this is how XML attributes work), so not really a big change.

@ryanemerson ryanemerson removed this from the 15.0.0.CR1 milestone Mar 4, 2024
@tristantarrant tristantarrant added this to the 15.0.1.Final milestone Mar 25, 2024
@tristantarrant tristantarrant force-pushed the ISPN-14568/cache_alias branch 2 times, most recently from 181c67b to d4b353c Compare March 28, 2024 14:22
@tristantarrant
Copy link
Member Author

Failures are unrelated

@tristantarrant tristantarrant removed this from the 15.0.1.Final milestone Apr 15, 2024
@tristantarrant tristantarrant added this to the 15.0.2.Final milestone Apr 15, 2024
@pruivo pruivo modified the milestones: 15.0.2.Final, 15.0.3.Final Apr 20, 2024
@tristantarrant
Copy link
Member Author

It's green!

@ryanemerson ryanemerson merged commit 5a5751c into infinispan:main May 3, 2024
9 checks passed
@ryanemerson
Copy link
Contributor

Thanks @tristantarrant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wburns wburns wburns left review comments

@fax4ever fax4ever fax4ever left review comments

@ryanemerson ryanemerson ryanemerson requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
15.0.3.Final
6 participants
@tristantarrant @ryanemerson @wburns @fax4ever @karesti @pruivo