[
https://issues.apache.org/jira/browse/ARTEMIS-3850?focusedWorklogId=781108&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-781108
]
ASF GitHub Bot logged work on ARTEMIS-3850:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 14/Jun/22 11:31
Start Date: 14/Jun/22 11:31
Worklog Time Spent: 10m
Work Description: gemmellr commented on code in PR #4101:
URL: https://github.com/apache/activemq-artemis/pull/4101#discussion_r896545097
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/EmptyList.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.utils.collections;
+
+import java.util.function.Consumer;
+
+public class EmptyList<E> implements LinkedList<E> {
+
+
+ private static final LinkedList EMPTY_LIST = new EmptyList();
+
+ public static final <T> LinkedList<T> getEmptyList() {
+ return (LinkedList<T>) EMPTY_LIST;
+ }
+
+ private EmptyList() {
+ }
+
+
+
+
+ @Override
+ public void addHead(E e) {
+ throw new UnsupportedOperationException("method not supported");
+ }
+
+ @Override
+ public void addTail(E e) {
+ throw new UnsupportedOperationException("method not supported");
+ }
+
+ @Override
+ public E get(int position) {
+ return null;
Review Comment:
It should probably throw IOOBE as the standard list should.
##########
tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/util/EmptyListTest.java:
##########
@@ -0,0 +1,38 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.tests.unit.util;
+
+import java.util.Iterator;
+
+import org.apache.activemq.artemis.utils.collections.EmptyList;
+import org.apache.activemq.artemis.utils.collections.LinkedList;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class EmptyListTest {
+
+ @Test
+ public void testEmpty() {
+ LinkedList<String> stringEmpty = EmptyList.getEmptyList();
+ Assert.assertEquals(0, stringEmpty.size());
Review Comment:
Could also test get on the empty list here.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/paging/PagingTest.java:
##########
@@ -209,12 +210,7 @@ public void testTooLongPageStoreTableNamePrefix() throws
Exception {
@Test
public void testPageOnLargeMessageMultipleQueues() throws Exception {
- internaltestOnLargetMessageMultipleQueues(MESSAGE_SIZE, true);
- }
-
- @Test
- public void testPageOnLargeMessageMultipleQueuesNoPersistence() throws
Exception {
- internaltestOnLargetMessageMultipleQueues(LARGE_MESSAGE_SIZE, false);
+ internaltestOnLargetMessageMultipleQueues(LARGE_MESSAGE_SIZE, true);
Review Comment:
You removed the other test using the boolean...so the boolean itself can go.
##########
tests/compatibility-tests/src/test/java/org/apache/activemq/artemis/tests/compatibility/MultiVersionReplicaTest.java:
##########
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.tests.compatibility;
+
+import javax.jms.Connection;
+import javax.jms.ConnectionFactory;
+import javax.jms.MessageConsumer;
+import javax.jms.MessageProducer;
+import javax.jms.Queue;
+import javax.jms.Session;
+import javax.jms.TextMessage;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory;
+import org.apache.activemq.artemis.tests.compatibility.base.ClasspathBase;
+import org.apache.qpid.jms.JmsConnectionFactory;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static
org.apache.activemq.artemis.tests.compatibility.GroovyRun.SNAPSHOT;
+import static
org.apache.activemq.artemis.tests.compatibility.GroovyRun.TWO_TWENTYTWO_ZERO;
+
+@RunWith(Parameterized.class)
+public class MultiVersionReplicaTest extends ClasspathBase {
+
+ private static final String QUEUE_NAME = "MultiVersionReplicaTestQueue";
+
+ private final String main;
+ private final ClassLoader mainClassloader;
+
+ private final String backup;
+ private final ClassLoader backupClassLoader;
+
+
+
+ @Parameterized.Parameters(name = "main={0}, backup={1}")
+ public static Collection getParameters() {
+ List<Object[]> combinations = new ArrayList<>();
+ combinations.add(new Object[]{TWO_TWENTYTWO_ZERO, SNAPSHOT});
+ combinations.add(new Object[]{SNAPSHOT, TWO_TWENTYTWO_ZERO});
+ combinations.add(new Object[]{SNAPSHOT, SNAPSHOT});
Review Comment:
I wonder about this last one. On the one hand, testing it works isnt really
bad...on the other, I would expect that should be covered elsewhere already and
so this is duplication, and it doesnt really fit the 'MultiVersionReplicaTest'
name.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -72,7 +66,11 @@ public final class PageSubscriptionImpl implements
PageSubscription {
private static final Logger logger =
Logger.getLogger(PageSubscriptionImpl.class);
- private static final PagedReference dummyPagedRef = new
PagedReferenceImpl(null, null, null);
+ private static final Object DUMMY = new Object();
+
+
+
Review Comment:
odd variable separation between fields
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1172,15 +1044,9 @@ public final class PageCursorInfo implements
ConsumedPage {
private final long pageId;
- // Confirmed ACKs on this page
- private Set<PagePosition> acks = Collections.synchronizedSet(new
LinkedHashSet<PagePosition>());
-
- private WeakReference<PageCache> cache;
+ private IntObjectHashMap<PagePosition> acks = new IntObjectHashMap<>();
- private Set<PagePosition> removedReferences = new ConcurrentHashSet<>();
-
- // The page was live at the time of the creation
- private final boolean wasLive;
+ private IntObjectHashMap removedReferences = new IntObjectHashMap<>();
Review Comment:
Should still be typed or wild carded to avoid the raw type warning it will
otherwise emit.
##########
tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/util/EmptyListTest.java:
##########
@@ -0,0 +1,38 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.tests.unit.util;
+
+import java.util.Iterator;
+
+import org.apache.activemq.artemis.utils.collections.EmptyList;
+import org.apache.activemq.artemis.utils.collections.LinkedList;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class EmptyListTest {
+
+ @Test
+ public void testEmpty() {
+ LinkedList<String> stringEmpty = EmptyList.getEmptyList();
+ Assert.assertEquals(0, stringEmpty.size());
+ Iterator<String> stringIterator = stringEmpty.iterator();
+ Assert.assertFalse(stringIterator.hasNext());
+ Assert.assertNull(stringIterator.next());
Review Comment:
This seems off, I would expect it to throw NoSuchElementException if there
is no element to return. Seems like a real list iterator would.
##########
tests/compatibility-tests/src/main/resources/multiVersionReplica/mainServer.groovy:
##########
@@ -0,0 +1,74 @@
+package multiVersionReplica
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import org.apache.activemq.artemis.api.core.QueueConfiguration
+import org.apache.activemq.artemis.api.core.RoutingType
+import org.apache.activemq.artemis.core.config.ClusterConnectionConfiguration
+import org.apache.activemq.artemis.core.config.CoreAddressConfiguration
+import org.apache.activemq.artemis.core.config.ha.ReplicatedPolicyConfiguration
+import org.apache.activemq.artemis.core.config.impl.ConfigurationImpl
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
Review Comment:
Ditto
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/EmptyList.java:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.utils.collections;
+
+import java.util.function.Consumer;
+
+public class EmptyList<E> implements LinkedList<E> {
+
+
+ private static final LinkedList EMPTY_LIST = new EmptyList();
+
+ public static final <T> LinkedList<T> getEmptyList() {
+ return (LinkedList<T>) EMPTY_LIST;
+ }
+
+ private EmptyList() {
+ }
+
+
+
+
+ @Override
+ public void addHead(E e) {
+ throw new UnsupportedOperationException("method not supported");
+ }
+
+ @Override
+ public void addTail(E e) {
+ throw new UnsupportedOperationException("method not supported");
+ }
+
+ @Override
+ public E get(int position) {
+ return null;
+ }
+
+ @Override
+ public E poll() {
+ return null;
+ }
+
+ LinkedListIterator<E> emptyIterator = new LinkedListIterator<E>() {
+ @Override
+ public void repeat() {
+ }
+
+ @Override
+ public void close() {
+ }
+
+ @Override
+ public boolean hasNext() {
+ return false;
+ }
+
+ @Override
+ public E next() {
+ return null;
Review Comment:
It should probably throw NoSuchElementException as a real list iterator
would seem to.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java:
##########
@@ -673,15 +738,24 @@ public Page removePage(int pageId) {
return null;
}
- Page page = createPage(pageId);
+ Page page = usePage(pageId);
- if (page.getFile().exists()) {
+ if (page != null && page.getFile().exists()) {
+ page.usageDown();
// we only decrement numberOfPages if the file existed
// it could have been removed by a previous delete
// on this case we just need to ignore this and move on
numberOfPages--;
}
+ logger.tracef("Removing paging %s, now containing
numberOfPages=%s", pageId, numberOfPages);
Review Comment:
page instead of paging?
##########
tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/util/LinkedListTest.java:
##########
@@ -1385,6 +1385,29 @@ public void testMultipleIterators2() {
}
+
+ @Test
+ public void testGetElement() {
+
+ for (int i = 0; i < 100; i++) {
+ list.addTail(i);
+ }
+
+ for (int i = 0; i < 100; i++) {
+ Assert.assertEquals(i, list.get(i).intValue());
+ }
+
+ boolean expectedException = false;
+
+ try {
+ list.get(300);
Review Comment:
Testing the boundary cases would be more obvious to better verify its
working as expected i.e 0 for empty list and 100 in this case.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageCursorProviderAccessor.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
Review Comment:
The \<p\> above and below should go, and URL be indented as expected, now
that this is a comment and not javadoc
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/ReplicationPageEventMessage.java:
##########
@@ -20,10 +20,13 @@
import org.apache.activemq.artemis.api.core.SimpleString;
import org.apache.activemq.artemis.core.protocol.core.impl.PacketImpl;
import org.apache.activemq.artemis.utils.DataConstants;
+import org.jboss.logging.Logger;
public class ReplicationPageEventMessage extends PacketImpl {
- private int pageNumber;
+ private static final Logger logger =
Logger.getLogger(ReplicationPageEventMessage.class);
Review Comment:
Logger is being added, but no noticable logging was. Unused?
##########
docs/user-manual/en/paging.md:
##########
@@ -192,6 +197,12 @@ The pages are synced periodically and the sync period is
configured through
the same value of `journal-buffer-timeout`. When using ASYNCIO, the default
should be `3333333`.
+## Memory usage from Paged Messages.
+
+The system should keep at least one paged file in memory caching ahead reading
messages.
+Also every active subscription could keep one paged file in memory. So if your
system has too many queues (some people would call this an horizontal topology).
+So, if your system has too many queues it is recommended to minimize the
page-size.
Review Comment:
```suggestion
The system should keep at least one paged file in memory caching ahead
reading messages.
Also every active subscription could keep one paged file in memory.
So, if your system has too many queues it is recommended to minimize the
page-size.
```
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java:
##########
@@ -673,15 +738,24 @@ public Page removePage(int pageId) {
return null;
}
- Page page = createPage(pageId);
+ Page page = usePage(pageId);
- if (page.getFile().exists()) {
+ if (page != null && page.getFile().exists()) {
+ page.usageDown();
Review Comment:
This goes on to do numberOfPages
Issue Time Tracking
-------------------
Worklog Id: (was: 781108)
Time Spent: 7h 40m (was: 7.5h)
> Add Option to read messages into paging based on sizing and eliminate caching
> -----------------------------------------------------------------------------
>
> Key: ARTEMIS-3850
> URL: https://issues.apache.org/jira/browse/ARTEMIS-3850
> Project: ActiveMQ Artemis
> Issue Type: New Feature
> Affects Versions: 2.22.0
> Reporter: Clebert Suconic
> Assignee: Clebert Suconic
> Priority: Major
> Fix For: 2.24.0
>
> Time Spent: 7h 40m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
