gemmellr commented on code in PR #4265:
URL: https://github.com/apache/activemq-artemis/pull/4265#discussion_r1047425947
##########
artemis-server/src/test/java/org/apache/activemq/artemis/tests/util/ActiveMQTestBase.java:
##########
@@ -2314,6 +2314,10 @@ protected int getMessageCount(final PostOffice
postOffice, final String address)
}
protected int getMessageCount(final Queue queue) {
+ try {
+ Wait.waitFor(() -> queue.getPageSubscription().isCounterPending() ==
false);
+ } catch (Exception ignroed) {
Review Comment:
typo
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingManagerImpl.java:
##########
@@ -548,4 +567,26 @@ public void lock() {
syncLock.writeLock().lock();
}
+ @Override
+ public void forEachTransaction(BiConsumer<Long, PageTransactionInfo>
transactionConsumer) {
+ transactions.forEach(transactionConsumer);
+ }
+
+ @Override
+ public Future<Object> rebuildCounters() {
+ LongHashSet transactionsSet = new LongHashSet();
+ transactions.forEach((txId, tx) -> {
+ transactionsSet.add(txId);
+ });
+ stores.forEach((address, pgStore) -> {
+ PageCounterRebuildManager rebuildManager = new
PageCounterRebuildManager(pgStore, transactionsSet);
+ logger.debug("Setting destination {} to rebuild counters",
pgStore.getAddress());
Review Comment:
Is the address arg passed to the lambda the same as pgStore.getAddress()
returns? If so use it instead?
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/paging/PageCountSyncOnNonTXTest.java:
##########
@@ -151,7 +156,7 @@ public void run() {
}
}
} catch (Exception expected) {
- expected.printStackTrace();
+ logger.warn(expected.toString(), expected);
Review Comment:
Debug? If its expected it hardly seems a warning. Message would be good to
say 'Got expected exception...' so it doesnt look like an unexpected one.
##########
tests/compatibility-tests/src/main/resources/pageCounter/sendMessages.groovy:
##########
@@ -0,0 +1,53 @@
+package pageCounter
+
+import org.apache.activemq.artemis.tests.compatibility.GroovyRun
+
+import javax.jms.*
+
+/*
+ * 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-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingManagerImpl.java:
##########
@@ -466,19 +471,25 @@ public void start() throws Exception {
reloadStores();
- if (ARTEMIS_DEBUG_PAGING_INTERVAL > 0) {
- this.scheduledComponent = new
ActiveMQScheduledComponent(pagingStoreFactory.getScheduledExecutor(),
pagingStoreFactory.newExecutor(), ARTEMIS_DEBUG_PAGING_INTERVAL,
TimeUnit.SECONDS, false) {
+ if (ARTEMIS_PAGING_COUNTER_SNAPSHOT_INTERVAL > 0) {
+ this.snapshotUpdater = new
ActiveMQScheduledComponent(pagingStoreFactory.getScheduledExecutor(),
pagingStoreFactory.newExecutor(), ARTEMIS_PAGING_COUNTER_SNAPSHOT_INTERVAL,
TimeUnit.SECONDS, false) {
@Override
public void run() {
- debug();
Review Comment:
The method is still present, perhaps unused, can/should it go too?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQMessageBundle.java:
##########
@@ -522,6 +522,9 @@ IllegalStateException invalidRoutingTypeUpdate(String
queueName,
@Message(id = 229243, value = "Embedded web server restart failed")
ActiveMQException embeddedWebServerRestartFailed(Exception e);
- @Message(id = 229244, value = "Meters already registered for {}")
+ @Message(id = 229244, value = "Management controller is busy with another
task. Please try again")
+ ActiveMQTimeoutException managementBusy();
+
+ @Message(id = 229245, value = "Meters already registered for {}")
Review Comment:
Changes an existing ID while also adding a new one, seems odd?
##########
tests/compatibility-tests/src/main/resources/pageCounter/checkMessages.groovy:
##########
@@ -0,0 +1,31 @@
+package pageCounter
+
+import org.apache.activemq.artemis.api.core.SimpleString
+import org.apache.activemq.artemis.core.server.Queue
+import org.apache.activemq.artemis.tests.compatibility.GroovyRun
+
+/*
+ * 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:
Header is in weird place, cant it go at/nearer top?
--
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]
