[fix][io] Fix Alluxio sink to respect the alluxioMasterHost property #19172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicoloboschi
approved these changes
Jan 10, 2023
|
/pulsarbot rerun-failure-checks |
tisonkun
approved these changes
Jan 10, 2023
Codecov Report
@@ Coverage Diff @@
## master #19172 +/- ##
============================================
+ Coverage 47.22% 47.40% +0.17%
+ Complexity 10713 9503 -1210
============================================
Files 713 633 -80
Lines 69697 59892 -9805
Branches 7485 6239 -1246
============================================
- Hits 32914 28391 -4523
+ Misses 33096 28383 -4713
+ Partials 3687 3118 -569
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Interesting. I'm trying to execute the test locally but always get: It seems Alluxio 2.7.3 doesn't support Apple M1 chip and thus the case. |
|
Merging... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently, the Alluxio sink always refers to localhost regardless of the
alluxioMasterHostproperty.Suppose we have a local Pulsar cluster (192.168.2.2) and a remote Alluxio cluster (192.168.33.10), as follows:
Build the Alluxio sink on the master branch and deploy it to the local Pulsar:
Enable the Alluxio sink and ingest some messages into the topic in question:
As the error messages indicate, the Alluxio sink tried to access the local address (192.168.2.2), in spite of the value of
alluxioMasterHost(192.168.33.10).Modifications
The reason of this behavior is in the following line.
https://github.com/apache/pulsar/blob/master/pulsar-io/alluxio/src/main/java/org/apache/pulsar/io/alluxio/sink/AlluxioSink.java#L95
The value of
alluxioMasterHostis set to the default configuration in that line, but the configuration object which is really used to createFileSystemis already instantiated before it, so it's ineffective.https://github.com/apache/pulsar/blob/master/pulsar-io/alluxio/src/main/java/org/apache/pulsar/io/alluxio/sink/AlluxioSink.java#L100
This PR fixes this wrong implementation.
Verifying this change
I manually ensured that the ingested messages were output as an Alluxio file with this PR, as follows:
Before ingesting messages:
During ingestion:
After ingestion:
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: sekikn#5
A few tests failed, but all of them seem to be unrelated to this fix.