dschneider-pivotal commented on a change in pull request #6814:
URL: https://github.com/apache/geode/pull/6814#discussion_r703794538
##########
File path:
geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/pubsub/Subscriptions.java
##########
@@ -96,104 +66,84 @@ boolean exists(Object channelOrPattern, Client client) {
* @param pattern the glob pattern to search for
*/
public List<byte[]> findChannelNames(byte[] pattern) {
-
- GlobPattern globPattern = new GlobPattern(bytesToString(pattern));
-
- return findChannelNames()
- .stream()
- .filter(name -> globPattern.matches(bytesToString(name)))
- .collect(Collectors.toList());
+ return channelSubscriptions.getIds(pattern);
}
/**
- * Return a list consisting of pairs {@code channelName, subscriptionCount}.
- *
- * @param names a list of the names to consider. This should not include any
patterns.
+ * Return a count of all pattern subscriptions including duplicates.
*/
- public List<Object> findNumberOfSubscribersPerChannel(List<byte[]> names) {
- List<Object> result = new ArrayList<>();
-
- names.forEach(name -> {
- Long subscriptionCount = findSubscriptions(name)
- .stream()
- .filter(subscription -> subscription instanceof ChannelSubscription)
- .count();
-
- result.add(name);
- result.add(subscriptionCount);
- });
+ public int getPatternSubscriptionCount() {
+ return patternSubscriptions.getSubscriptionCount();
+ }
- return result;
+ @VisibleForTesting
+ int getChannelSubscriptionCount() {
+ return channelSubscriptions.getSubscriptionCount();
}
- /**
- * Return a count of all pattern subscriptions including duplicates.
- */
- public long findNumberOfPatternSubscriptions() {
- return subscriptions.stream()
- .filter(subscription -> subscription instanceof PatternSubscription)
- .count();
+ void add(ChannelSubscription subscription) {
+ channelSubscriptions.add(subscription);
}
- /**
- * Add a new subscription
- */
- @VisibleForTesting
- void add(Subscription subscription) {
- subscriptions.add(subscription);
+ void add(PatternSubscription subscription) {
+ patternSubscriptions.add(subscription);
}
/**
* Remove all subscriptions for a given client
*/
public void remove(Client client) {
- subscriptions.removeIf(subscription -> subscription.matchesClient(client));
- }
-
- /**
- * Remove a single subscription
- */
- @VisibleForTesting
- boolean remove(Object channel, Client client) {
- return subscriptions.removeIf(subscription ->
subscription.isEqualTo(channel, client));
+ channelSubscriptions.remove(client);
+ patternSubscriptions.remove(client);
+ client.clearSubscriptions();
}
/**
* @return the total number of all local subscriptions
*/
@VisibleForTesting
int size() {
- return subscriptions.size();
+ // this is only used by tests so performance is not an issue
+ return getChannelSubscriptionCount() + getPatternSubscriptionCount();
}
- public synchronized SubscribeResult subscribe(byte[] channel,
ExecutionHandlerContext context,
- Client client) {
- Subscription createdSubscription = null;
- if (!exists(channel, client)) {
- createdSubscription = new ChannelSubscription(client, channel, context,
this);
+ public SubscribeResult subscribe(byte[] channel, ExecutionHandlerContext
context) {
+ final Client client = context.getClient();
+ ChannelSubscription createdSubscription = null;
+ if (client.addChannelSubscription(channel)) {
+ createdSubscription = new ChannelSubscription(channel, context, this);
add(createdSubscription);
}
- long channelCount = findSubscriptions(client).size();
+ long channelCount = client.getSubscriptionCount();
return new SubscribeResult(createdSubscription, channelCount, channel);
}
- public SubscribeResult psubscribe(byte[] patternBytes,
ExecutionHandlerContext context,
- Client client) {
- GlobPattern pattern = new GlobPattern(bytesToString(patternBytes));
- Subscription createdSubscription = null;
- synchronized (this) {
- if (!exists(pattern, client)) {
- createdSubscription = new PatternSubscription(client, pattern,
context, this);
+ public SubscribeResult psubscribe(byte[] patternBytes,
ExecutionHandlerContext context) {
+ final Client client = context.getClient();
+ PatternSubscription createdSubscription = null;
+ if (client.addPatternSubscription(patternBytes)) {
+ boolean added = false;
+ try {
+ createdSubscription = new PatternSubscription(patternBytes, context,
this);
add(createdSubscription);
+ added = true;
+ } finally {
+ if (!added) {
+ // Must have had a problem parsing the pattern
Review comment:
We have always used the jdk Pattern.compile to parse our glob patterns.
It will throw PatternSyntaxException for various reasons. I added a unit test
to cover this particular code and found this pattern would throw:
stringToBytes("\\C"). Note this is just a back slash byte followed by a 'C'
byte.
I did not introduce in this pr any of the code in GlobPattern that uses the
jdk Pattern.compile but I do wonder if this is an area in which we differ with
native redis. When we take the input pattern and convert it to a jdk regex
pattern we should probably be turning a single back slash into a double back
slash (i.e. a literal backslash) unless it is followed by one of the special
characters of glob pattern (I gave native redis a "\C" pattern and it treated
it like "C"). I will file a ticket about this. Perhaps our GlobPattern class
should never submit a pattern to the jdk that will throw
PatternSyntaxException. Once that is done then this code would not need to
worry about cleaning up if the add fails. See GEODE-9582
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
