[fix][client] Fix LoadManagerReport not found

[nodece]

Motivation

When using the pulsar-client-all and pulsar-client-admin libraries, the admin.brokerStats().getLoadReport() doesn't work:

DEBUG o.a.p.c.util.ObjectMapperFactory - Add LoadManagerReport deserializer failed because LoadManagerReport.class has been shaded
java.lang.ClassNotFoundException: org.apache.pulsar.shade.org.apache.pulsar.policies.data.loadbalancer.LoadManagerReport
2025-01-23 16:15:16.816 [taskScheduler-9] DEBUG o.a.p.c.util.ObjectMapperFactory - Add LoadManagerReport deserializer failed because LoadManagerReport.class has been shaded
java.lang.ClassNotFoundException: org.apache.pulsar.shade.org.apache.pulsar.policies.data.loadbalancer.LoadManagerReport
2025-01-23 16:15:28.232 [http-nio-10000-exec-1] DEBUG o.s.w.s.m.m.a.ExceptionHandlerExceptionResolver - Resolved [org.springframework.web.util.NestedServletException: Handler dispatch failed; nested exception is java.lang.AbstractMethodError: Receiver class org.apache.pulsar.client.admin.internal.BrokerStatsImpl does not define or inherit an implementation of the resolved method 'abstract org.apache.pulsar.policies.data.loadbalancer.LoadManagerReport getLoadReport()' of interface org.apache.pulsar.client.admin.BrokerStats.]

We got this error because the shade plugin relocates the org.apache.pulsar.policies.data.loadbalancer.* package, and org.apache.pulsar.policies.data.loadbalancer.LoadManagerReport didn't added to the shaded-jar.

Modifications

• Relocating the org.apache.pulsar.policies is meaningless, so remove that from the shade configuration.

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

[nodece]

/pulsarbot rerun-failure-checks

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.24%. Comparing base (bbc6224) to head (89e0d49).
Report is 868 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #23886      +/-   ##
============================================
+ Coverage     73.57%   74.24%   +0.67%     
+ Complexity    32624    32241     -383     
============================================
  Files          1877     1853      -24     
  Lines        139502   143634    +4132     
  Branches      15299    16317    +1018     
============================================
+ Hits         102638   106642    +4004     
+ Misses        28908    28623     -285     
- Partials       7956     8369     +413     

Flag	Coverage Δ
inttests	26.73% <100.00%> (+2.14%)	⬆️
systests	23.12% <100.00%> (-1.20%)	⬇️
unittests	73.76% <100.00%> (+0.92%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...apache/pulsar/common/util/ObjectMapperFactory.java	95.65% <100.00%> (+1.96%)	⬆️

... and 1024 files with indirect coverage changes

[lhotari]

It looks like that shading changes should be reverted and there should be logic in LoadReportDeserializer to handle the unshaded classname that comes from the broker:

pulsar/pulsar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/LoadReportDeserializer.java

Line 39 in 798a014

if ((root.has("loadReportType") && root.get("loadReportType").asText().equals(LoadReport.loadReportType))

suggested fix

    public LoadManagerReport deserialize(JsonParser jsonParser, DeserializationContext deserializationContext)
            throws IOException, JsonProcessingException {
        ObjectReader reader = ObjectMapperFactory.getMapper().reader();
        ObjectNode root = reader.readTree(jsonParser);
        String loadReportType = LoadReport.loadReportType;
        // remove possible shading prefix
        String loadReportTypeUnshaded = LoadReport.loadReportType
                .replaceFirst("^.+\\." + Pattern.quote(LoadReport.class.getPackageName()),
                        LoadReport.class.getPackageName());
        if ((root.has("loadReportType")
                && (root.get("loadReportType").asText().equals(loadReportType))
                || root.get("loadReportType").asText().equals(loadReportTypeUnshaded))
                || (root.has("underLoaded"))) {
            return reader.treeToValue(root, LoadReport.class);
        } else {
            return reader.treeToValue(root, LocalBrokerData.class);
        }
    }

[nodece]

It looks like that shading changes should be reverted and there should be logic in LoadReportDeserializer to handle the unshaded classname that comes from the broker:

Both the interface (org.apache.pulsar.policies.data.loadbalancer.LoadManagerReport) and its implementation are being shaded, but the interface class isn’t being properly added to the shaded package. This mismatch is what’s causing the class resolution issue when using the shaded jar.

To resolve this, we should ensure that the interface is also included in the shaded package, keeping it consistent with the implementation. Alternatively, as a simpler solution, we could consider removing that shading the org.apache.pulsar.policies, which would eliminate these classpath problems.

@lhotari What's your opinion?

[nodece]

BTW, it works fine in our private pulsar. We are using the pulsar-client-all to feat the broker resource(broker-stats/topic/namespace/subscript/so on) info.

[lhotari]

@lhotari What's your opinion?

@nodece I've suggested a fix in #23886 (review) . Have you checked that?

[lhotari]

Both the interface (org.apache.pulsar.policies.data.loadbalancer.LoadManagerReport) and its implementation are being shaded, but the interface class isn’t being properly added to the shaded package. This mismatch is what’s causing the class resolution issue when using the shaded jar.

To resolve this, we should ensure that the interface is also included in the shaded package, keeping it consistent with the implementation. Alternatively, as a simpler solution, we could consider removing that shading the org.apache.pulsar.policies, which would eliminate these classpath problems.

Public interfaces shouldn't be shaded. Implementation classes should be shaded. That's the original intent.
The mistake has been to use the classname in org.apache.pulsar.policies.data.loadbalancer.LoadReport#loadReportType field. Shading will change that to be the shaded class name and as a result of this, the deserialization will fail since there's a difference between client side and broker side for the value of the loadReportType field. This problem is addressed by the changes that I suggested in #23886 (review).

[lhotari]

The interface classes present here should be excluded from shading: https://github.com/apache/pulsar/tree/master/pulsar-client-admin-api/src/main/java/org/apache/pulsar/policies/data/loadbalancer

Something like

<relocation>
  <pattern>org.apache.pulsar.policies</pattern>
  <shadedPattern>org.apache.pulsar.shade.org.apache.pulsar.policies</shadedPattern>
  <excludes>
    <exclude>org\.apache\.pulsar\.policies\.data\.loadbalancer\.(LoadManagerReport|NamespaceBundleStats|ResourceUsage|ServiceLookupData)($.*)?</exclude>
  </excludes>
</relocation>

[lhotari]

I added some more details about how to resolve the problem.

[nodece]

@lhotari What's your opinion?

@nodece I've suggested a fix in #23886 (review) . Have you checked that?

This is an error fix, I still got the same error.

org.apache.pulsar.shade.org.apache.pulsar.policies.data.loadbalancer.LoadManagerReport does not exist.

Public interfaces shouldn't be shaded. Implementation classes should be shaded.

I understand that this is the correct way. Your changes are correct.

Shading will change that to be the shaded class name and as a result of this, the deserialization will fail since there's a difference between client side and broker side for the value of the loadReportType field.

    public static final String loadReportType = LoadReport.class.getSimpleName();

The loadReportType value is a simple name, so we don't need to consider the full name, and this means LoadReportDeserializer should not modified.

  <excludes>
    <!-- exclude references to unshaded classes and interfaces in https://github.com/apache/pulsar/tree/master/pulsar-client-admin-api/src/main/java/org/apache/pulsar/policies/data/loadbalancer -->   
    <exclude>org\.apache\.pulsar\.policies\.data\.loadbalancer\.(LoadManagerReport|NamespaceBundleStats|ResourceUsage|ServiceLookupData)($.*)?</exclude>
  </excludes>

It doesn't work, so I use the following code instead of that:

<excludes>
  <exclude>org.apache.pulsar.policies.data.loadbalancer.LoadManagerReport</exclude>
  <exclude>org.apache.pulsar.policies.data.loadbalancer.NamespaceBundleStats</exclude>
  <exclude>org.apache.pulsar.policies.data.loadbalancer.ResourceUsage</exclude>
  <exclude>org.apache.pulsar.policies.data.loadbalancer.ServiceLookupData</exclude>
</excludes>

@lhotari Thank you!

[lhotari]

LGTM

[lhotari]

The loadReportType value is a simple name, so we don't need to consider the full name, and this means LoadReportDeserializer should not modified.

@nodece Oh yes, missed that detail.