Skip to content

AMQ-9452: unwind BaseDestination to access queue/topic message#1174

Merged
jbonofre merged 1 commit into
apache:mainfrom
ggkochanski:AMQ-9452
Mar 22, 2024
Merged

AMQ-9452: unwind BaseDestination to access queue/topic message#1174
jbonofre merged 1 commit into
apache:mainfrom
ggkochanski:AMQ-9452

Conversation

@ggkochanski
Copy link
Copy Markdown

When dest is instance of DestinationFilter the queue/topic message is not accessed (statistic "firstMessageTimestamp" is not sent)

@jbonofre jbonofre self-requested a review March 14, 2024 09:17
@mattrpav mattrpav self-requested a review March 14, 2024 15:56
Copy link
Copy Markdown
Contributor

@mattrpav mattrpav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test, and squash the commits into one for this change.

Change-Id: Ic05002ecb428e2aa5abeb9dc3e499e3aae550051
@ggkochanski
Copy link
Copy Markdown
Author

@mattrpav I've added changes in BrokerStatisticsPluginTest which proves the fix:

  1. additional plugin, which just proxes Destination with DestinationFilter:
        //AMQ-9452: proxy destinations with DestinationFilter
        plugins[0] = new BrokerPluginSupport() {
            @Override
            public Set<Destination> getDestinations(ActiveMQDestination destination) {
                return super.getDestinations(destination).stream().map(DestinationFilter::new).collect(Collectors.toSet());
            }
        };
  1. additional assert in test testDestinationStatsWithFirstMessageTimestamp, which proves statistic field firstMessageTimestamp is set by StatisticsBroker:
    assertTrue(reply.getLong("firstMessageTimestamp") > 0);

@jbonofre
Copy link
Copy Markdown
Member

@ggkochanski Thanks ! I will take a look.

@ggkochanski ggkochanski requested a review from mattrpav March 16, 2024 05:24
@ggkochanski
Copy link
Copy Markdown
Author

really nice metric (tried patched version in action), I encourage to merge 😊

image

@jbonofre jbonofre merged commit 45bce62 into apache:main Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants