Return safe fallback from BrokerView read-only getters instead of throwing when broker not yet started

[jbonofre]

Summary

• Fixes the Jolokia/JMX IllegalStateException reported in Jolokia/JMX queries fail with IllegalStateException on slave broker when startAsync=true #2551: on a slave broker running with startAsync=true, querying the Broker MBean over Jolokia failed even though attributes like Slave were perfectly readable, because Jolokia reads all attributes in one batch and a single throwing getter aborted the whole response.
• BrokerView's read-only attribute getters (getBrokerId(), getBrokerName(), destination/subscriber/producer lists, and the destination statistics counters) now return a safe fallback (null/0/empty array, broker name falls back to the configured name on BrokerService) instead of throwing via safeGetBroker() when ManagedRegionBroker hasn't been set yet.
• Getters that mutate broker state (resetStatistics(), enableStatistics(), disableStatistics(), destination add/remove, durable subscriber create/destroy) are unchanged and still throw IllegalStateException through safeGetBroker(), since they genuinely can't run before the broker is up.

Test plan

• mvn -pl activemq-broker -am compile succeeds
• Manual verification: start a slave broker with startAsync=true and confirm a Jolokia bulk attribute read on the Broker MBean succeeds (including Slave=true) while the broker is still waiting on the lock

Fixes #2551

[cshannon]

Instead of the null check here, it might make more sense to check the state of the broker and see if it's the active broker or the standby. If it's the active broker, and the broker object is null, that would mean a real error and the exception should be thrown.

[jbonofre]

@cshannon on slave the broker bean is null. I can do an indirect accessor but I think null check is good enough for the slave case (the purpose here).

[mattrpav]

I have a concern that changing the exception handling would be breaking for JMX clients that already handle the exception being thrown.

I’ll do a couple of checks

I prefer this clean/empty data approach over throwing exceptions, but it may need to wait to go into a major version.

[jbonofre]

@mattrpav nothing is break, it just bypass the exception when broker is not there, the exceptions are still thrown as before on "active" broker.

[jbonofre]

@cshannon the exceptions are still thrown when broker is not null (so on an active broker).

[cshannon]

@cshannon on slave the broker bean is null. I can do an indirect accessor but I think null check is good enough for the slave case (the purpose here).

I was thinking you could do something like this:

@Override
public long getTotalConsumerCount() {
    if (isStandBy()) {
        return 0;
    }
    return safeGetBroker().getDestinationStatistics().getConsumers().getCount();
}

private boolean isStandBy() {
    return broker == null && brokerService.isSlave();
}

BrokerService should never be null (even if broker is)

[jbonofre]

@cshannon I'm fine to do that but not sure it changes anything.

I've tested the BrokerService:

1. slave defaults to true and is reset to true on every stop.
2. It only flips to false inside startAllConnectors().
3. adminView.setBroker(managedBroker) (this call makes BrokerView.broker non-null) happens in doStartBroker() which runs before startAllConnectors().

So within a single BrokerService instance, broker == null implies isSlave() == true at every point at startup (there's no case where broker is null but isSlave() has already gone false.

It means:

isStandBy() = (broker == null) && brokerService.isSlave()

is correct but && isSlave() term can never actually be false while the first is true, given the current ordering in BrokerService.

So:

• It changes no behavior
• It adds a dependency on the slave for a "guard" whose only real job is null-safety before calling safeGetBroker().
• If an user ever reorders startAllConnectors()/adminView.setBroker() in BrokerService (unlikely, but possible), this guard behavior would silently change without anyone touching BrokerView.

I'm fine to add isSlave() check, even if I believe it's useless 😄

[jbonofre]

@mattrpav that makes totally sense. Maybe we should target just 6.4.0 for this change (not 7)?

[cshannon]

@mattrpav that makes totally sense. Maybe we should target just 6.4.0 for this change (not 7)?

+1 for 6.4.0

I think this change is probably fine for something like 6.4.0. It also depends on if we want to use the empty data approach instead of error if broker is null no matter what (like the existing PR) or limit it to only if in slave mode. (the broker object is not set until start is called so i'm not sure if it's possible for the broker to be null briefly during initialization). I suggested to do that extra check but maybe we just leave things as is, not sure.

[cshannon]

@cshannon I'm fine to do that but not sure it changes anything.

I've tested the BrokerService:

1. slave defaults to true and is reset to true on every stop.
2. It only flips to false inside startAllConnectors().
3. adminView.setBroker(managedBroker) (this call makes BrokerView.broker non-null) happens in doStartBroker() which runs before startAllConnectors().

So within a single BrokerService instance, broker == null implies isSlave() == true at every point at startup (there's no case where broker is null but isSlave() has already gone false.

Alright, based on that it's probably no point to add the check. As you stated it doesn't really buy us anything.

[cshannon]

So after looking it over and thinking about it, I think the current version is fine for 6.4.0, but not a backport due to the change to empty vs throwing an exception. We can make a note in the release notes because I think this is overall a better way to handle it.