gaogaotiantian commented on PR #57247:
URL: https://github.com/apache/spark/pull/57247#issuecomment-4975117502
I'm a bit confused about the purpose and design. I have a few questions that
are probably in different area.
1. Why is `pid` not part of the contract? What benefit do we gain from not
having it? I saw an extra private small class required for daemon workers
because we don't have the `pid` in the new class. Do we expect worker handles
that are not 1:1 to os process?
2. The design is a bit twisted in the following ways:
a. `pid` is not part of contract, but the handle is created by `of(pid)`
- it's still part of the contract.
b. `PythonWorkerHandle` is an abstract class with the awareness of
`faulthandler`, which is an implementation detail for current Python workers.
What is the clear interface of it? If it knows about `faulthandler`, why should
the logging method and switch passed to it externally? Why can't we just set up
everything when we create the handle? Or should we design the interface in a
more general way that is irrelevant to `faulthandler`?
c. Why is `DaemonWorker` even needed? Just because `pid` is not part of
the contract?
3. What's the next step? Because currently there's not duplicated code
removed. We basically just changed everything and add some extra code which
makes things a bit more complicated.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
