Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][broker] normalize path #23438

Merged
merged 1 commit into from
Oct 11, 2024
Merged

[fix][broker] normalize path #23438

merged 1 commit into from
Oct 11, 2024

Conversation

nodece
Copy link
Member

@nodece nodece commented Oct 11, 2024

Motivation

In the broker configuration, we're using relative paths for parameters such as brokerInterceptorsDirectory, protocolHandlerDirectory, additionalServletDirectory, and offloadersDirectory. These paths include the ./ notation, and since the broker doesn't normalize them, the absolute paths generated are incorrect, for example: /app/pulsar/20241011165422044/./interceptors/pulsar-xxx.nar. As a result, the broker is unable to resolve the correct file locations.

Test cases:

    @Test
    public void testPathNormalize() {
        Path path = Paths.get("./interceptors").toAbsolutePath().normalize();
        assertEquals(path.toString().indexOf("./"), -1);
    }

    @Test
    public void testPathWithoutNormalize() {
        Path path = Paths.get("./interceptors").toAbsolutePath();
        assertNotEquals(path.toString().indexOf("./"), -1);
    }

Modifications

  • Append the java.nio.file.Path#normalize call after the java.nio.file.Path#toAbsolutePath call to ensure proper path resolution.

Documentation

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

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece self-assigned this Oct 11, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 11, 2024
@lhotari
Copy link
Member

lhotari commented Oct 11, 2024

These paths include the ./ notation, and since the broker doesn't normalize them, the absolute paths generated are incorrect, for example: /app/pulsar/20241011165422044/./interceptors/pulsar-xxx.nar. As a result, the broker is unable to resolve the correct file locations.

I'm not against making this change. Just one question: Technically a path like /app/pulsar/20241011165422044/./interceptors/pulsar-xxx.nar is correct. What is the error message in this case?

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@nodece
Copy link
Member Author

nodece commented Oct 11, 2024

59778ea0-ce0c-47ee-aa1d-4a0efb710c53

I'm sure the $PULSAR_HOME/interceptors/message-trace-impl-impl-2.10.7.1.nar exists.

Changed the brokerInterceptorsDirectory value from . /interceptors to interceptors, it works fine.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 74.32%. Comparing base (bbc6224) to head (1b4d35d).
Report is 663 commits behind head on master.

Files with missing lines Patch % Lines
...ker/web/plugin/servlet/AdditionalServletUtils.java 50.00% 1 Missing ⚠️
...ulsar/broker/intercept/BrokerInterceptorUtils.java 50.00% 1 Missing ⚠️
...sar/broker/service/plugin/EntryFilterProvider.java 50.00% 1 Missing ⚠️
.../functions/worker/service/WorkerServiceLoader.java 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23438      +/-   ##
============================================
+ Coverage     73.57%   74.32%   +0.75%     
- Complexity    32624    34452    +1828     
============================================
  Files          1877     1953      +76     
  Lines        139502   147177    +7675     
  Branches      15299    16204     +905     
============================================
+ Hits         102638   109389    +6751     
- Misses        28908    29358     +450     
- Partials       7956     8430     +474     
Flag Coverage Δ
inttests 27.61% <17.64%> (+3.02%) ⬆️
systests 24.38% <23.52%> (+0.06%) ⬆️
unittests 73.67% <76.47%> (+0.82%) ⬆️

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

Files with missing lines Coverage Δ
...che/bookkeeper/mledger/offload/OffloaderUtils.java 63.23% <100.00%> (ø)
...e/pulsar/broker/protocol/ProtocolHandlerUtils.java 71.42% <100.00%> (ø)
.../admin/cli/utils/CustomCommandFactoryProvider.java 70.00% <100.00%> (ø)
.../java/org/apache/pulsar/functions/LocalRunner.java 50.82% <100.00%> (ø)
...ulsar/functions/utils/functions/FunctionUtils.java 77.41% <100.00%> (ø)
...ache/pulsar/functions/utils/io/ConnectorUtils.java 36.11% <100.00%> (ø)
.../pulsar/proxy/extensions/ProxyExtensionsUtils.java 71.42% <100.00%> (ø)
...ker/web/plugin/servlet/AdditionalServletUtils.java 31.57% <50.00%> (ø)
...ulsar/broker/intercept/BrokerInterceptorUtils.java 32.14% <50.00%> (ø)
...sar/broker/service/plugin/EntryFilterProvider.java 52.58% <50.00%> (ø)
... and 1 more

... and 633 files with indirect coverage changes

@nodece nodece merged commit 50dc521 into apache:master Oct 11, 2024
59 checks passed
@nodece nodece added this to the 4.0.0 milestone Oct 11, 2024
nodece added a commit that referenced this pull request Oct 11, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 50dc521)
@lhotari
Copy link
Member

lhotari commented Oct 11, 2024

nodece added this to the 4.0.0 milestone 3 minutes ago

@nodece Please don't modify the milestone. It will mess up tracking since the code freeze has already started. I can cherry-pick this to branch-4.0. Please read https://lists.apache.org/thread/xtnd51oy8ytmbtrctpmlpg2cqpjgf957

nodece added a commit that referenced this pull request Oct 11, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 50dc521)
@nodece nodece removed this from the 4.0.0 milestone Oct 11, 2024
@nodece nodece deleted the normalize-path branch October 11, 2024 15:49
@lhotari lhotari added this to the 4.0.0 milestone Oct 11, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Oct 15, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 50dc521)
(cherry picked from commit e884749)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Oct 16, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 50dc521)
(cherry picked from commit e884749)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants