CommandLookupTopic.advertisedListenerName may be blank

[EronWright]

Describe the bug
Some of the language bindings supply an empty string for the advertisedListenerName field of CommandLookupTopic (e.g. Java vs Golang). This confuses the broker, who then fails to apply the default advertised listener for the given server port. This, in turn, leads to an incorrect topic lookup response and a subsequent connection failure.

For background, see [PIP 95] Smart Listener Selection with Multiple Bind Addresses.

To Reproduce
Steps to reproduce the behavior:

1. Configure a broker with a bind address that is associated with an advertised listener.
2. Using various language clients, send a topic lookup request to the bind address, without specifying an advertised listener. Expect the default advertised listener for that binding to be used.
3. Observe that some clients receive the correct advertised listener, others don't.

Expected behavior
Expect the binding-specific advertised listener to be automatically applied to lookup request.

Proposed Solution
Enhance the broker to check for an empty string in the advertisedListenerName field. (code)

[michaeljmarshall]

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java

Lines 820 to 822 in 2d09237

	if (StringUtils.isNotBlank(listenerName)) {
	lookup.setAdvertisedListenerName(listenerName);
	}

@EronWright - were you able to reproduce this issue with the Java client? Based on the code you reference, it looks like the advertisedListenerName would be null unless a "non blank" string is used.

I can see in the Go client, we are using string type instead of a string reference (*string) type in the lookupService struct, and that results in passing an empty string.

I agree with your proposed solution, but I am concerned that this is a point of ambiguity in the spec that should be better defined. I'd also like to understand how we're seeing this issue with the Java client since it looks like it hasAdvertisedListenerName should return false in the ServerCnx.

[EronWright]

@michaeljmarshall thanks for the review. I believe that hasAdvertisedListenerName would return true for any value, including null.

I'm trying to write a test into ServerCnxTest, may be challenging, stand by.

[michaeljmarshall]

I believe that hasAdvertisedListenerName would return true for any value, including null.

I don't believe this is correct. If you look at the protobuf field definition, the advertised_listener_name field is an optional string.

pulsar/pulsar-common/src/main/proto/PulsarApi.proto

Line 437 in a43fab0

optional string advertised_listener_name = 7;

I just ran a quick test to double check my understanding, and this test passes:

    @Test
    public void testAdvertisedListenerName() {
        BaseCommand bc1 = new BaseCommand();
        bc1.setLookupTopic();
        assertFalse(bc1.getLookupTopic().hasAdvertisedListenerName());

        try {
            bc1.setLookupTopic().setAdvertisedListenerName(null);
            fail();
        } catch (NullPointerException e) {
            // expected
        }

        BaseCommand bc2 = new BaseCommand();
        bc2.setLookupTopic().setAdvertisedListenerName("");
        assertTrue(bc2.getLookupTopic().hasAdvertisedListenerName());
    }

[merlimat]

It will return true if the field is set. That is irrespective of the specific value that was set.

[EronWright]

@BewareMyPower please cherry-pick this fix to 2.9, thanks.

[BewareMyPower]

done.