Make sure ufs to only once load a native shared library #14058
Conversation
… (.so) at runtime Signed-off-by: Xue Yantao [email protected]
Codecov Report
@@ Coverage Diff @@
## master #14058 +/- ##
============================================
+ Coverage 43.29% 43.34% +0.05%
- Complexity 9171 9230 +59
============================================
Files 1417 1425 +8
Lines 82084 82518 +434
Branches 9938 9984 +46
============================================
+ Hits 35538 35771 +233
- Misses 43584 43797 +213
+ Partials 2962 2950 -12
Continue to review full report at Codecov.
|
| * @param factories list of factories | ||
| * @return list of factories that support the given path which may be an empty list | ||
| */ | ||
| public List<T> select(String path, S conf, List<T> factories) { |
There was a problem hiding this comment.
do we need this method to be public?
There was a problem hiding this comment.
do we need this method to be public?
may be not. private is better
| * @return list of factories that support the given path which may be an empty list | ||
| */ | ||
| public List<T> select(String path, S conf, List<T> factories) { | ||
| Preconditions.checkArgument(path != null, "path may not be null"); |
There was a problem hiding this comment.
alluxio code convention:
| Preconditions.checkArgument(path != null, "path may not be null"); | |
| Preconditions.checkNotNull(path, "path"); |
| return eligibleFactories; | ||
| } | ||
|
|
||
| factories.clear(); |
There was a problem hiding this comment.
i don't understand why we can prevent loading native shared lib multiple times by calling clear here.
can you elaborate?
There was a problem hiding this comment.
i don't understand why we can prevent loading native shared lib multiple times by calling clear here.
can you elaborate?
At this line, we did not find any supported factories from the list of factories. So we clear it to avoid to twicely check them wether support or not.
… (.so) at runtime Signed-off-by: Xue Yantao [email protected]
|
#10928 Not sure it is same root cause with this issue. |
… (.so) at runtime Signed-off-by: Xue Yantao [email protected]
| @@ -131,19 +131,18 @@ private synchronized void init() { | |||
| Preconditions.checkArgument(path != null, "path may not be null"); | |||
|
|
|||
| List<T> factories = new ArrayList<>(mFactories); | |||
| List<T> eligibleFactories = select(path, conf, factories); | |||
There was a problem hiding this comment.
You mean the first time findAll invoked, the mFactories should be empty, and this method will run like original logic, later new findAll will not load the extension or lib jar at all? So please show the effect.
There was a problem hiding this comment.
You mean the first time
findAllinvoked, themFactoriesshould be empty, and this method will run like original logic, later newfindAllwill not load the extension or lib jar at all? So please show the effect.
Yes, the new 'findAll' will not dynamicly load the ext or lib jar after the first load done. However, we can restart the alluxio master or worker to load the new updated jar.
There was a problem hiding this comment.
Please highlight add this important limitation to the related issue or the description of this PR. It would be better if you write an approach description shortly to let reviewer know your solution and make it easy to review this PR
| @@ -31,7 +31,6 @@ | |||
| <groupId>io.github.opendataio</groupId> | |||
| <artifactId>libcephfs</artifactId> | |||
| <version>0.0.1</version> | |||
| <scope>provided</scope> | |||
There was a problem hiding this comment.
What's the purpose you remove the provided scope, you want to include the libcephfs and related share lib into the ufs shaded with dependencies jar?
There was a problem hiding this comment.
What's the purpose you remove the
providedscope, you want to include thelibcephfsand related share lib into the ufs shaded with dependencies jar?
Yes, this pr will solve the problem about the libcephfs_jni.so is already loaded in another classloader, so libcephfs can be included.
|
The native shared library was loaded in another classloader: Below is the master log in my test: 2021-09-18 15:49:21,444 WARN UnderFileSystem$Factory - Failed to create UnderFileSystem by factory alluxio.underfs.cephfs.CephFSUnderFileSystemFactory@5e2694c1: java.lang.UnsatisfiedLinkError: Native Library /usr/lib64/libcephfs_jni.so.1.0.0 already loaded in another classloader |
|
After used this pr: the native shared lib problem is solved We test underfs-cephfs and underfs-cephfs-hadoop |
|
@apc999 PTAL |
|
Mainly saw in Ceph, C++ library |
| @@ -201,6 +200,7 @@ private void scan(List<File> files, List<T> factories) { | |||
| LOG.debug("Discovered a factory implementation {} - {} in jar {}", factory.getClass(), | |||
| factory, jarPath); | |||
| register(factory, factories); | |||
| register(factory); | |||
There was a problem hiding this comment.
/**
* The base list of factories, which does not include any lib or extension factories. The only
* factories in the base list will be built-in factories, and any additional factories
* registered by tests. All other factories will be discovered and service loaded when extension
* creation occurs.
*/
private final List<T> mFactories = new CopyOnWriteArrayList<>();
we modify the basic assumption of what mFactories represents
will it be better to include a new list call mDynamicLoadedFactories?
There was a problem hiding this comment.
- If mFactories contains -> return
- If mDynamicLoadedFactories contains -> return
- If not, renew the mDynamicLoadedFactories?
There was a problem hiding this comment.
@apc999 What's your thoughts
There was a problem hiding this comment.
- If mFactories contains -> return
- If mDynamicLoadedFactories contains -> return
- If not, renew the mDynamicLoadedFactories?
Yea, i think it sounds better! By the way, may be that we can add a method "boolean supportsDynamicLoad()" in the public interface ExtensionFactory to distinguish the underFileSystemFactories, the default return is true, likes the existed method "boolean supportsPath(String path, S conf)"，however, the specific underFileSystemFactories which do not support dynamic load can override the method, so we can still only use mFactories.
What's your thoughts? @apc999 @LuQQiu @maobaolong
There was a problem hiding this comment.
I prefer this way, but is it complicated to add the logic?
There was a problem hiding this comment.
I prefer this way, but is it complicated to add the logic?
Based on your idea, the steps are :
1. If mFactories contains && !supportsDynamicLoad() -> return
2. If mDynamicLoadedFactories contains && !supportsDynamicLoad() -> return
3. If not, renew the mDynamicLoadedFactories?
There was a problem hiding this comment.
@jiacheliu3 I think we need to make an agreement on how to address this issue. This part is kind of critical and nobody really familiar with it. More eyes need to be pulled in to think of how to load the extension factories
|
/ping @LuQQiu |
|
Sorry for the late review. @jhonxue can you help merge the latest master into the branch to solve the conflicts? |
ok, my pleasure. Also done the conflicts |
| @@ -159,6 +165,7 @@ public List<T> findAllWithRecorder(String path, S conf, Recorder recorder) { | |||
| recorder.record("alluxio.underfs.version is not set by user"); | |||
| } | |||
|
|
|||
| eligibleFactories = select(path, conf, factories); | |||
| for (T factory : factories) { | |||
There was a problem hiding this comment.
is select duplicate with the following for loop
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
github-actions
bot
removed
the
stale
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
github-actions
bot
removed
the
stale
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
Fixes #14142
What changes are proposed in this pull request?
Make sure ufs to only once load a native shared library(.so) at runtime, through making sure the same classloader for the same ufs
Why are the changes needed?
Otherwise cephfs mount will failed cuz loading same native library for multiple times
Does this PR introduce any user facing changes?
N/A
Signed-off-by: Xue Yantao [email protected]