[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[GitHub] activemq-artemis pull request #2228: ARTEMIS-2017 SelectorParser cache not t...

Github user franz1981 commented on a diff in the pull request:

    --- Diff: artemis-selector/src/main/java/org/apache/activemq/artemis/selector/impl/SelectorParser.java ---
    @@ -80,11 +78,15 @@ public static BooleanExpression parse(String sql) throws FilterException {
                    StrictParser parser = new StrictParser(new StringReader(actual));
                    e = parser.JmsSelector();
    -            cache.put(sql, e);
    +            synchronized (cache) {
    --- End diff --
    @jbertram @michaelandrepearce If it is not an hot path I thing that what Justin is suggesting makes totally sense: anyway if we can afford to put guava in place I believe that https://google.github.io/guava/releases/16.0/api/docs/com/google/common/cache/CacheBuilder.html#maximumSize(long) would be a good option too.
    It will scale with a factor specified by the concurrent level specified, but it could (I do not remember TBH) introduce some behavioural changes from the current impl.
    I understand both @michaelandrepearce and @jbertram opinions so I think that the general rule to balance changes vs effectiveness on common case apply here: I leave you decide how much this tradeoff apply here...
    I'm honored to be "summoned" :+1: