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-- but might the Page still remain in 'usedPages'? If it ever got in there, is it only going to be removed from there if page.usageDown() drops the count to 0 and causes the release task to fire and remvoe it? Yet usePage() increments its usage on each return. ########## artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java: ########## @@ -626,6 +656,41 @@ public Page createPage(final int pageNumber) throws Exception { return page; } + @Override + public final Page usePage(final long pageId) { + return usePage(pageId, true); + } + + @Override + public Page usePage(final long pageId, final boolean create) { + synchronized (usedPages) { + try { + Page page = usedPages.get(pageId); + if (create && page == null) { + page = newPageObject(pageId); + if (page.getFile().exists()) { + page.getMessages(); + injectPage(page); + } Review Comment: What is usedPages meant to track exactly? Might just be a naming thing, but previous times through I hadnt actually noticed that 'usedPages' doesnt necessarily have the Page returned from 'usePage()' added/injected to it, which feels a bit odd. Seems like usedPages is really being used as something more like **re**usedPages? ########## tests/compatibility-tests/src/main/resources/multiVersionReplica/backupServer.groovy: ########## @@ -0,0 +1,79 @@ +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.ReplicaPolicyConfiguration +import org.apache.activemq.artemis.core.config.ha.ReplicatedPolicyConfiguration +import org.apache.activemq.artemis.core.config.ha.ReplicationBackupPolicyConfiguration +import org.apache.activemq.artemis.core.config.ha.ReplicationPrimaryPolicyConfiguration +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: one copy seems sufficient? ########## artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java: ########## @@ -522,8 +538,22 @@ protected void reloadLivePage(int pageId) throws Exception { } } + private void resetCurrentPage(Page newCurrentPage) { + if (this.currentPage != null) { + this.currentPage.usageDown(); + } + + if (newCurrentPage != null) { + newCurrentPage.usageUp(); + injectPage(newCurrentPage); + } + + this.currentPage = newCurrentPage; + } Review Comment: Ordinarly this would seem unsafe as currentPage is marked volatile and it will read more than once, so its value should be assigned to a variable before null-check-then-using...but it seems this method is only called under write lock when starting, so it should get away with it. I'd update it anyway. ########## artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PageReadWriter.java: ########## @@ -0,0 +1,281 @@ +/** + * 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.core.paging.impl; + +import java.nio.ByteBuffer; +import java.util.function.Consumer; + +import io.netty.buffer.ByteBuf; +import io.netty.buffer.Unpooled; +import org.apache.activemq.artemis.api.core.ActiveMQBuffer; +import org.apache.activemq.artemis.api.core.SimpleString; +import org.apache.activemq.artemis.core.buffers.impl.ChannelBufferWrapper; +import org.apache.activemq.artemis.core.io.SequentialFile; +import org.apache.activemq.artemis.core.io.SequentialFileFactory; +import org.apache.activemq.artemis.core.paging.PagedMessage; +import org.apache.activemq.artemis.core.persistence.StorageManager; +import org.apache.activemq.artemis.core.server.LargeServerMessage; +import org.apache.activemq.artemis.utils.DataConstants; +import org.apache.activemq.artemis.utils.Env; +import org.jboss.logging.Logger; + +public class PageReadWriter { + + + private static Logger logger = Logger.getLogger(PageReadWriter.class); + + public static final int SIZE_RECORD = DataConstants.SIZE_BYTE + DataConstants.SIZE_INT + DataConstants.SIZE_BYTE; + + private static final byte START_BYTE = (byte) '{'; + + private static final byte END_BYTE = (byte) '}'; + + //sizeOf(START_BYTE) + sizeOf(MESSAGE LENGTH) + sizeOf(END_BYTE) + private static final int HEADER_AND_TRAILER_SIZE = DataConstants.SIZE_INT + 2; + private static final int MINIMUM_MSG_PERSISTENT_SIZE = HEADER_AND_TRAILER_SIZE; + private static final int HEADER_SIZE = HEADER_AND_TRAILER_SIZE - 1; + private static final int MIN_CHUNK_SIZE = Env.osPageSize(); + + public interface SuspectFileCallback { + void onSuspect(String fileName, int position, int msgNumber); + } + + public interface PageRecordFilter { + boolean skip(ActiveMQBuffer buffer); + } + + public interface ReadCallback { + void readComple(int size); + } + + public static final PageRecordFilter ONLY_LARGE = (buffer) -> !PagedMessageImpl.isLargeMessage(buffer); + + public static final PageRecordFilter NO_SKIP = (buffer) -> false; + + public static final PageRecordFilter SKIP_ALL = (buffer) -> true; + + public static int writeMessage(PagedMessage message, SequentialFileFactory fileFactory, SequentialFile file) throws Exception { + final int messageEncodedSize = message.getEncodeSize(); + final int bufferSize = messageEncodedSize + SIZE_RECORD; + final ByteBuffer buffer = fileFactory.newBuffer(bufferSize); + ChannelBufferWrapper activeMQBuffer = new ChannelBufferWrapper(Unpooled.wrappedBuffer(buffer)); + activeMQBuffer.clear(); + activeMQBuffer.writeByte(START_BYTE); + activeMQBuffer.writeInt(messageEncodedSize); + message.encode(activeMQBuffer); + activeMQBuffer.writeByte(END_BYTE); + assert (activeMQBuffer.readableBytes() == bufferSize) : "messageEncodedSize is different from expected"; + //buffer limit and position are the same + assert (buffer.remaining() == bufferSize) : "buffer position or limit are changed"; + file.writeDirect(buffer, false); + return bufferSize; + } + + + Review Comment: I realise you were jsut moving code, though the comment was more about the odd variable triple/double/other-spacing between the methods, which didnt exist in Page. -- 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]
