[SPARK-31769][CORE] Add MDC support for driver threads #28629
Conversation
|
cc @cloud-fan |
|
Add a test case? |
|
shall we at least provide one default driver side MDC property like app id？ |
|
@cloud-fan Do you have an idea where in the code is the first place we can add it? I thought to add in def getOrCreate(): SparkSession = synchronized {
assertOnDriver()
// Get the session from current thread's active session.
var session = activeThreadSession.get()
if ((session ne null) && !session.sparkContext.isStopped) {
applyModifiableSettings(session)
MDC.put("appName", session.conf.get("spark.app.name", ""))
MDC.put("appId", session.conf.get("spark.app.id", ""))
return session
} |
|
@keypointt Added test case |
Sounds good. It's better if you can do a local test, and show that we can see app id in the driver log. |
|
@cloud-fan Do we have tests that read logs? |
|
I mean manual test: just check the logs by yourself. I have a new question: how can we have multiple applications in one Spark driver? IIUC one Spark driver is dedicated to one Spark application, so it seems unnecessary to have the app id in the driver log. |
|
@cloud-fan I think you are right, it can't be. so maybe we don't need to add this as default. |
|
ping @cloud-fan what do you think. |
|
I'm OK to have this framework, but without any builtin driver MDC properties. Please find a way to test it. |
|
@cloud-fan What exactly you want to test? |
|
e.g. you set a custom MDC property via |
dongjoon-hyun
changed the title
|
|
We don't need an automatic test, just test it manually and put the result in the PR description. And yes, it's not |
|
@cloud-fan MDC.put("appName", "app for test")
val session = SparkSession.builder()
.master("local")
.config(UI_ENABLED.key, value = false)
.config("some-config", "v2")
.getOrCreate()log4j.properties: #File Appender with MDC
log4j.appender.FAMDC=org.apache.log4j.FileAppender
log4j.appender.FAMDC.append=false
log4j.appender.FAMDC.file=target/unit-tests-mdc.log
log4j.appender.FAMDC.layout=org.apache.log4j.PatternLayout
log4j.appender.FAMDC.layout.ConversionPattern=%d{HH:mm:ss.SSS} [%X{appId}] [%X{appName}] %t %p %c{1}: %m%nlog file: |
|
yea test is useful. Now I have one question: for task MDC properties, they are always prefixed with |
|
the driver inherits the MDC from calling thread so I think from user perspective it is not convenient as in most cases user will want to use its regular keys not create a dedicated one. |
|
But inconsistency is also bad. @Ngone51 do you have any ideas? |
|
Maybe, we could provide API for the user to add MDC properties and restrict the format of the MDC key? For example, |
|
BTW, what's wrong with reusing the |
|
we can add support of adding MDC to the driver using |
|
Retest this please |
|
Test build #124571 has finished for PR 28629 at commit
|
|
Retest this please. |
|
Test build #124575 has finished for PR 28629 at commit
|
|
@dongjoon-hyun the failed test does not seems to be connected to the code changes. |
|
|
||
| test("newDaemonSingleThreadExecutor with MDC") { | ||
| val appIdValue = s"appId-${System.currentTimeMillis()}" | ||
| MDC.put("appId", appIdValue) |
There was a problem hiding this comment.
I'm still worried about API inconsistency. We should use either sc.setLocalProperties or MDC.put for both driver and executor side MDC properties.
There was a problem hiding this comment.
@cloud-fan I think he is using MDC.put in both cases. The only difference is that for executor we also have to call sc.setLocalProperties in order to pass the property into the executor JVM. Once the executor process has knowledge of the appId or any other property, it is put into MDC the same way.
There was a problem hiding this comment.
I mean user-facing APIs. Eventually, we have to call MDC.put as this is how MDC works.
Maybe it's better to say that Spark can propagate the MDC properties to both driver thread pools and executor tasks. So end-users only need to call MDC.put.
There was a problem hiding this comment.
For the driver is true. for the Executor as it is in another process it should be called explicitly by the user.
There was a problem hiding this comment.
right. there is no way to make MDC put something in another process.
|
Let's think about the use cases. There are many users who share one spark application (e.g. job server), and each user can set MDC properties so that he can collect spark logs for his spark jobs only. (correct me if I misunderstand the use case) If I were an end-user, I'd like to use one API to set MDC properties for both driver and executor logs. I don't see a use case that needs driver and executor have different MDC properties (except that the executor has an extra builtin MDC property: the task id). I agree that it's better to just inherit the MDC properties. And that should be the story for end-users. Spark should do two things internally:
This PR does 1, but 2 is only partially done. We shouldn't ask users to manually set the local properties, but we should do it for them: when submitting a job, read the MDC properties of the current thread and set it to local properties. Another problem is the MDC property names. Since we just inherit it at driver-side, it's not possible to require the "mdc." prefix. Maybe we should also remove this pre-fix at the executor side. cc @Ngone51 |
|
I'm fine to remove the prefix if we want to inherit the MDC properties directly since I agree API consistent is more important. And I think we need to document it clearly that which MDC users should use as Spark actually has Besides, I have two questions: I just realize that MDC is actually backed by the And it seems this PR also can't work with the loop running runnable, e.g. |
|
@cloud-fan, can we continue with Adding the Driver inherited MDC passing to Executor using |
SGTM |
|
looking at the code I thought to add the properties in line: 1216-1219 // Use the scheduling pool, job group, description, etc. from an ActiveJob associated
// with this Stage
val properties = jobIdToActiveJob(jobId).properties
addPySparkConfigsToProperties(stage, properties)What do you think? |
|
can you add a new commit? It's easier to review on github |
|
@cloud-fan, What should we do if |
|
Good question. Since we expect end-users to set MDC properties directly, I think MDC properties should take priority over local properties. The local properties is just an internal detail about how we propagate driver side MDC properties to executor side. I think this should be rare as we have the |
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
|
Why is it not merged into the master |
|
@l568288g that PR was intend to be in 3.1.X so 2.3 is not relevant in any case. |
|
Is it ok to just modify ThreadUtils? Does it need to be changed to MDCAwareRunnable where Runnable is used? |
thank you for your reply |
11 participants
What changes were proposed in this pull request?
Adds MDC propagate into all driver threads so all the logs can use it.
Why are the changes needed?
The use case is applicable in 2 cases:
Does this PR introduce any user-facing change?
No
How was this patch tested?
Run all tests.
Manual:
code:
log4j.properties:
log file: