gemmellr commented on code in PR #4260:
URL: https://github.com/apache/activemq-artemis/pull/4260#discussion_r998150771
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -832,13 +832,14 @@ public boolean isPersistent() {
@Override
public void processReload() throws Exception {
if (recoveredACK != null) {
- logger.trace("********** processing reload!!!!!!!");
+ logger.debug("processing reload queue name={} with id={}", queue !=
null ? this.queue.getName() : "N/A", this.cursorId);
Review Comment:
The cursorId value is a long, so you should now gate the logger to avoid
boxing. The "this" qualifiers dont seem needed.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java:
##########
@@ -883,17 +883,12 @@ protected Map<String, Object>[] getFirstMessage() throws
Exception {
clearIO();
try {
List<Map<String, Object>> messages = new ArrayList<>();
- queue.flushExecutor();
final int attributeSizeLimit =
addressSettingsRepository.getMatch(address).getManagementMessageAttributeSizeLimit();
- try (LinkedListIterator<MessageReference> iterator =
queue.browserIterator()) {
- // returns just the first, as it's the first only
- if (iterator.hasNext()) {
- MessageReference ref = iterator.next();
- Message message = ref.getMessage();
- messages.add(message.toMap(attributeSizeLimit));
- }
- return messages.toArray(new Map[1]);
+ MessageReference firstMessage = queue.peekFirstMessage();
+ if (firstMessage != null) {
+ messages.add(firstMessage.getMessage().toMap(attributeSizeLimit));
}
+ return messages.toArray(new Map[1]);
Review Comment:
If the idea is to optimise this overall getFirstMessage() method, then there
still looks to be room for improvement here. It creates an ArrayList of Map,
then afterwards figures out whether it will put 0 or 1 things in it, causing
creation of a new array of capacity 10 if there is, and then converts the
resulting ArrayList of 0/1 items into a new created array of size 1.
Seems like it could just omit the ArrayList and create the final array, then
if the map value exists add it, then return it. Or if allowed by the API and
its uses, even just only create the array once its known there is something to
put in it, and return null otherwise.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1460,23 +1473,47 @@ private PagedReference moveNext() {
private PagedReference internalGetNext() {
for (;;) {
+ if (logger.isDebugEnabled() && currentPageIterator == null) {
+ logger.debug("Page Cursor {} has no pageIterator", cursorId,
new Exception("trace"));
Review Comment:
'tracing exception stacktrace' at debug seems odd.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1278,11 +1286,16 @@ private class CursorIterator implements PageIterator {
private LinkedListIterator<PagedMessage> currentPageIterator;
private void initPage(long page) {
+ logger.debug("initPage {}", page);
Review Comment:
should be gated
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1460,23 +1473,47 @@ private PagedReference moveNext() {
private PagedReference internalGetNext() {
for (;;) {
+ if (logger.isDebugEnabled() && currentPageIterator == null) {
Review Comment:
Is the null value unlikely to happen? Seems like it since it will presumably
NPE below if so. (EDIT: so do we even need the logging?)
If its the rarer event, I'd put its check first. I'd probably just make it 2
ifs.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1460,23 +1473,47 @@ private PagedReference moveNext() {
private PagedReference internalGetNext() {
for (;;) {
+ if (logger.isDebugEnabled() && currentPageIterator == null) {
+ logger.debug("Page Cursor {} has no pageIterator", cursorId,
new Exception("trace"));
+ }
PagedMessage message = currentPageIterator.hasNext() ?
currentPageIterator.next() : null;
logger.trace("CursorIterator::internalGetNext:: new reference {}",
message);
if (message != null) {
return cursorProvider.newReference(message,
PageSubscriptionImpl.this);
}
- if (currentPage.getPageId() < pageStore.getCurrentWritingPage()) {
+ if (logger.isTraceEnabled()) {
+ logger.trace("Current page {}", currentPage != null ?
currentPage.getPageId() : null);
+ }
+ long nextPage = getNextPage();
+ if (logger.isTraceEnabled()) {
+ logger.trace("next page {}", nextPage);
+ }
+ if (nextPage >= 0) {
if (logger.isTraceEnabled()) {
- logger.trace("CursorIterator::internalGetNext:: moving to
currentPage {}", currentPage.getPageId() + 1);
+ logger.trace("CursorIterator::internalGetNext:: moving to
currentPage {}", nextPage);
}
- initPage(currentPage.getPageId() + 1);
+ initPage(nextPage);
} else {
return null;
}
}
}
+ private long getNextPage() {
+ long page = currentPage.getPageId() + 1;
+
+ while (page <= pageStore.getCurrentWritingPage()) {
+ PageCursorInfo info = locatePageInfo(page);
+ if (info == null || info.getCompleteInfo() == null) {
+ return page;
+ }
+ logger.debug("Subscription {} named {} moving faster from page {}
to next", cursorId, queue.getName(), page);
Review Comment:
Triple arg, and long values...add gate
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1278,11 +1286,16 @@ private class CursorIterator implements PageIterator {
private LinkedListIterator<PagedMessage> currentPageIterator;
private void initPage(long page) {
+ logger.debug("initPage {}", page);
try {
if (currentPage != null) {
+ if (logger.isTraceEnabled()) {
+ logger.trace("usage down {} on subscription {}",
currentPage.getPageId(), cursorId);
+ }
currentPage.usageDown();
}
if (currentPageIterator != null) {
+ logger.trace("closing pageIterator on {}", cursorId);
Review Comment:
should be gated
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -3164,6 +3174,7 @@ private void checkDepage() {
return;
}
if (pageIterator != null && pageSubscription.isPaging()) {
+ logger.debug("CheckDepage on queue name {}, id={}", this.name,
this.id);
Review Comment:
The "this." qualifiers seem unnecessary.
--
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]
