You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/12/23 08:05:10 UTC
[GitHub] [iceberg] yittg opened a new pull request #3797: Sync thread context class loader with where invoked for ThreadPools
yittg opened a new pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797
Fixes #3776
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yittg commented on pull request #3797: Sync thread context class loader with where invoked for ThreadPools
Posted by GitBox <gi...@apache.org>.
yittg commented on pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797#issuecomment-1014123021
> let's get the fix for manifest file reading in and then address the next issue.
@rdblue I've open #3906 for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3797: Sync thread context class loader with where invoked for ThreadPools
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797#issuecomment-1011572583
@yittg, I'm skeptical that this is the right approach. The dynamic class loader allows you to specify the classloader to use. It is generally better to specify the correct one instead of relying on the context loader.
In this case, it looks like the [reader used to load the file's key-value metadata](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/ManifestReader.java#L100-L102) doesn't have the class loader set like the [reader used to scan the manifest](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/ManifestReader.java#L207). I would first try setting the class loader the same way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yittg commented on pull request #3797: Sync thread context class loader with where invoked for ThreadPools
Posted by GitBox <gi...@apache.org>.
yittg commented on pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797#issuecomment-1007074772
@rdblue Do you think this make sense? Or what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3797: Sync thread context class loader with where invoked for ThreadPools
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797#issuecomment-1013493417
@yittg, let's get the fix for manifest file reading in and then address the next issue. I think that we should not be relying on the context classloader when loading dynamically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yittg commented on pull request #3797: Sync thread context class loader with where invoked for ThreadPools
Posted by GitBox <gi...@apache.org>.
yittg commented on pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797#issuecomment-1011659358
> In this case, it looks like the [reader used to load the file's key-value metadata](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/ManifestReader.java#L100-L102) doesn't have the class loader set like the [reader used to scan the manifest](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/ManifestReader.java#L207). I would first try setting the class loader the same way.
@rdblue It's also my first choice, FYI https://github.com/apache/iceberg/issues/3776#issuecomment-998502908, but it's not enough, as described in my comment, you can not pass the correct one to all cases in the dependencies.
> For example, the follow code snippet in [java.xml](https://github.com/openjdk/jdk/blob/jdk-11%2B12/src/java.xml/share/classes/javax/xml/parsers/FactoryFinder.java#L285)
> ```
> final ServiceLoader<T> serviceLoader = ServiceLoader.load(type);
> ```
IMO, the right way is not to use the static shared thread pool. But it's hard to fix here, the shared pool is involved in too many places.
So i choose this quick fix to walk around it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yittg commented on pull request #3797: Sync thread context class loader with where invoked for ThreadPools
Posted by GitBox <gi...@apache.org>.
yittg commented on pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797#issuecomment-1011670623
@rdblue I think it's also ok to set the correct placeholder in iceberg, i can open another PR to address it, then the issue can be fixed in those scenarios not touching the same issue in third-party dependencies.
Then we can think about how to fix it thoroughly.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yittg closed pull request #3797: Fixed context classloader of threads in ThreadPools
Posted by GitBox <gi...@apache.org>.
yittg closed pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3797: Sync thread context class loader with where invoked for ThreadPools
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797#issuecomment-1003202958
@yittg can you please explain what this is doing and why it is the correct behavior?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yittg commented on pull request #3797: Fixed context classloader of threads in ThreadPools
Posted by GitBox <gi...@apache.org>.
yittg commented on pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797#issuecomment-1046531282
close this one due to prefer #4177
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yittg commented on pull request #3797: Fixed context classloader of threads in ThreadPools
Posted by GitBox <gi...@apache.org>.
yittg commented on pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797#issuecomment-1016203006
@rdblue i updated changes with fixed context class loader.
looks like ci failed accidentally,
retest ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yittg commented on pull request #3797: Sync thread context class loader with where invoked for ThreadPools
Posted by GitBox <gi...@apache.org>.
yittg commented on pull request #3797:
URL: https://github.com/apache/iceberg/pull/3797#issuecomment-1003249667
@rdblue The context of thread in ThreadPool inherited from the parent thread. For the first task of one thread, the parent thread is current thread that executes the task, so the context is consistent with where the task is executed. As of the second and following tasks, the context of the existing thread running the task is still the one that started the thread, which can be different from the one that executed the task. And this can cause the context class loader issue described in #3776 .
For example:
1. Thread[A] with Context[Ca] executes Task[T1];
2. ThreadPool starts new Thread[P1] with Context[Ca], and running Task[T1], (OK);
3. Thread[B] with Context[Cb] executes Task[T2];
4. ThreadPool using Thread[P1] with Context[Ca] to run the Task[T2], (not OK);
> Context[Ca] can be different from Context[Cb] and maybe already expired now.
This change records the context that executes one task in the wrapper Runnable, and sets it to the target thread before actually running the task. This can make sure the consistency of the context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org