Skip to content

Commit a267532

Browse files
committed
Java SDK: Change coordinator comm to use multiplexing
Serializing each client's whole send → receive → deserialize round trip behind one lock (#69080) allows only a single request in flight at a time, so a task issuing concurrent calls over the shared comm socket pays a full, serialized round trip per call. Every frame already carries a request id, so a single background dispatcher can read the socket and route each response by id, with the lock guarding only the write. Signed-off-by: PoAn Yang <payang@apache.org>
1 parent 963d286 commit a267532

6 files changed

Lines changed: 491 additions & 84 deletions

File tree

‎java-sdk/sdk/src/main/kotlin/org/apache/airflow/sdk/Server.kt‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@ import kotlinx.coroutines.runBlocking
3333
import org.apache.airflow.sdk.execution.CoordinatorComm
3434
import org.apache.airflow.sdk.execution.LogSender
3535
import org.apache.airflow.sdk.execution.Logger
36+
import org.apache.airflow.sdk.execution.comm.ErrorResponse
37+
import org.apache.airflow.sdk.execution.comm.StartupDetails
38+
import org.apache.airflow.sdk.execution.runTask
3639
import kotlin.text.substringAfterLast
3740
import kotlin.text.substringBeforeLast
3841

@@ -147,10 +150,11 @@ class Server(
147150
aSocket(SelectorManager(Dispatchers.IO)).tcp().connect(comm).use { socket ->
148151
logger.debug("Connected comm", mapOf("addr" to comm))
149152
CoordinatorComm(
150-
bundle,
151153
socket.openReadChannel(),
152154
socket.openWriteChannel(autoFlush = true),
153-
).startProcessing()
155+
).use { coordinator ->
156+
dispatchTask(bundle, coordinator)
157+
}
154158
}
155159
} finally {
156160
deferral.complete(Unit)
@@ -164,4 +168,25 @@ class Server(
164168
}
165169
}
166170
}
171+
172+
internal suspend fun dispatchTask(
173+
bundle: Bundle,
174+
coordinator: CoordinatorComm,
175+
) {
176+
val frame = coordinator.readMessage()
177+
when (val body = frame.body) {
178+
is StartupDetails -> runTaskAndReport(bundle, body, coordinator)
179+
is ErrorResponse -> throw ApiError("[${body.error}] ${body.detail}")
180+
else -> throw ApiError("Unexpected initial frame (id=${frame.id})")
181+
}
182+
}
183+
184+
private suspend fun runTaskAndReport(
185+
bundle: Bundle,
186+
startup: StartupDetails,
187+
coordinator: CoordinatorComm,
188+
) {
189+
val result = runTask(bundle, startup, coordinator)
190+
coordinator.communicate<Unit>(result)
191+
}
167192
}

‎java-sdk/sdk/src/main/kotlin/org/apache/airflow/sdk/execution/Comm.kt‎

Lines changed: 114 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,18 @@ import io.ktor.utils.io.ByteReadChannel
2323
import io.ktor.utils.io.ByteWriteChannel
2424
import io.ktor.utils.io.readByteArray
2525
import io.ktor.utils.io.writeByteArray
26+
import kotlinx.coroutines.CancellationException
27+
import kotlinx.coroutines.CompletableDeferred
28+
import kotlinx.coroutines.CoroutineScope
29+
import kotlinx.coroutines.Dispatchers
30+
import kotlinx.coroutines.SupervisorJob
31+
import kotlinx.coroutines.cancel
32+
import kotlinx.coroutines.launch
2633
import kotlinx.coroutines.sync.Mutex
2734
import kotlinx.coroutines.sync.withLock
2835
import org.apache.airflow.sdk.ApiError
29-
import org.apache.airflow.sdk.Bundle
3036
import org.apache.airflow.sdk.execution.comm.ErrorResponse
31-
import org.apache.airflow.sdk.execution.comm.StartupDetails
37+
import java.util.concurrent.ConcurrentHashMap
3238
import kotlin.concurrent.atomics.AtomicInt
3339
import kotlin.concurrent.atomics.ExperimentalAtomicApi
3440

@@ -44,10 +50,9 @@ data class OutgoingFrame(
4450

4551
@OptIn(ExperimentalAtomicApi::class)
4652
class CoordinatorComm(
47-
private val bundle: Bundle,
4853
private val reader: ByteReadChannel,
4954
private val writer: ByteWriteChannel,
50-
) {
55+
) : AutoCloseable {
5156
internal companion object {
5257
private val logger = Logger(CoordinatorComm::class)
5358

@@ -57,77 +62,63 @@ class CoordinatorComm(
5762
}
5863

5964
private val nextId = AtomicInt(0)
60-
private var shutDownRequested = false
61-
private val commMutex = Mutex()
65+
private val writeMutex = Mutex()
66+
private val stateMutex = Mutex()
67+
private val pending = ConcurrentHashMap<Int, CompletableDeferred<IncomingFrame>>()
6268

63-
suspend fun startProcessing() {
64-
while (!shutDownRequested) {
65-
processOnce(::handleIncoming)
66-
}
67-
logger.debug("Goodbye")
68-
}
69+
@Volatile
70+
private var readError: ApiError? = null
71+
private var dispatcherStarted = false
6972

70-
private suspend fun processOnce(handle: suspend (IncomingFrame) -> Unit) {
71-
val prefix = reader.readByteArray(4) // First 4 bytes as length.
72-
if (prefix.size != 4) { // Something is terribly wrong. Let's bail.
73-
logger.error("Need 4 prefix bytes", mapOf("actual" to prefix.size))
74-
shutDownRequested = true
75-
return
76-
}
73+
private val dispatcherScope = CoroutineScope(Dispatchers.IO + SupervisorJob())
7774

78-
val payloadLength = Frame.parseLengthPrefix(prefix)
79-
val payload = reader.readByteArray(payloadLength)
80-
if (payload.size != payloadLength) { // Something is terribly wrong. Let's bail.
81-
logger.error(
82-
"Payload length not right",
83-
mapOf("expect" to payloadLength, "receive" to payload.size),
84-
)
85-
shutDownRequested = true
86-
return
75+
suspend fun readMessage(): IncomingFrame =
76+
stateMutex.withLock {
77+
check(!dispatcherStarted) { "readMessage cannot be used after the dispatcher has started" }
78+
readFrame()
8779
}
88-
val frame = decode(payload)
89-
logger.debug("Handling", mapOf("id" to frame.id))
90-
handle(frame)
91-
}
9280

9381
private suspend fun sendMessage(
9482
id: Int,
9583
body: Any,
9684
) {
9785
val data = encode(OutgoingFrame(id, body))
9886
logger.debug("Sending", mapOf("id" to id, "body" to body))
99-
writer.writeByteArray(Frame.lengthPrefix(data.size))
100-
writer.writeByteArray(data)
101-
}
102-
103-
suspend fun handleIncoming(frame: IncomingFrame) {
104-
when (val request = frame.body) {
105-
null -> {}
106-
is ErrorResponse -> throw ApiError("[${request.error}] ${request.detail}")
107-
is StartupDetails -> {
108-
communicate<Unit>(runTask(bundle, request, this))
109-
shutDownRequested = true
110-
}
87+
writeMutex.withLock {
88+
writer.writeByteArray(Frame.lengthPrefix(data.size) + data)
11189
}
11290
}
11391

11492
@Throws(ApiError::class)
11593
suspend fun communicateImpl(body: Any): Any {
11694
val requestId = nextId.fetchAndAdd(1)
117-
return commMutex.withLock {
118-
var frame: IncomingFrame? = null
95+
val waiter = CompletableDeferred<IncomingFrame>()
11996

120-
suspend fun handle(f: IncomingFrame) {
121-
frame = f
97+
stateMutex.withLock {
98+
readError?.let { throw it }
99+
if (!dispatcherStarted) {
100+
dispatcherStarted = true
101+
dispatcherScope.launch { readLoop() }
122102
}
103+
pending[requestId] = waiter
104+
}
105+
106+
// A close() concurrent with the block above either drains this waiter (it
107+
// was registered first) or is visible here, so no caller is left awaiting a
108+
// comm that is already gone.
109+
readError?.let {
110+
pending.remove(requestId)
111+
throw it
112+
}
113+
114+
try {
123115
sendMessage(requestId, body)
124-
processOnce(::handle)
125-
val received = frame ?: throw ApiError("No response received")
126-
if (received.id != requestId) {
127-
throw ApiError("response id ${received.id} does not match request id $requestId")
128-
}
129-
received.body ?: Unit
116+
} catch (e: Throwable) {
117+
pending.remove(requestId)
118+
throw e
130119
}
120+
121+
return waiter.await().body ?: Unit
131122
}
132123

133124
@Throws(ApiError::class)
@@ -138,4 +129,73 @@ class CoordinatorComm(
138129
else -> throw ApiError("Unexpected response type ${response::class.java}")
139130
}
140131
}
132+
133+
/**
134+
* Stop the dispatcher and fail anything still awaiting a response.
135+
*/
136+
override fun close() {
137+
dispatcherScope.cancel()
138+
failAllWaiters(readError ?: ApiError("Coordinator comm closed"))
139+
}
140+
141+
private suspend fun readPayload(): ByteArray {
142+
val prefix = reader.readByteArray(4) // First 4 bytes as length.
143+
if (prefix.size != 4) {
144+
throw ApiError("Coordinator socket closed while reading frame length")
145+
}
146+
val payloadLength = Frame.parseLengthPrefix(prefix)
147+
val payload = reader.readByteArray(payloadLength)
148+
if (payload.size != payloadLength) {
149+
throw ApiError("Coordinator socket closed while reading frame payload")
150+
}
151+
return payload
152+
}
153+
154+
private suspend fun readFrame(): IncomingFrame {
155+
val frame = decode(readPayload())
156+
logger.debug("Received", mapOf("id" to frame.id))
157+
return frame
158+
}
159+
160+
private suspend fun readLoop() {
161+
while (true) {
162+
val raw =
163+
try {
164+
Frame.decodeRaw(readPayload())
165+
} catch (e: CancellationException) {
166+
// Coroutine cancellation is delivered by throwing this exception, and
167+
// cooperative cancellation requires rethrowing it so it propagates.
168+
throw e
169+
} catch (e: Throwable) {
170+
failAllWaiters(e)
171+
return
172+
}
173+
logger.debug("Received", mapOf("id" to raw.id))
174+
175+
val waiter = pending.remove(raw.id)
176+
if (waiter == null) {
177+
logger.warning("Discarding response with no matching request", mapOf("id" to raw.id))
178+
continue
179+
}
180+
181+
try {
182+
waiter.complete(IncomingFrame(raw.id, Frame.decodeBody(raw)))
183+
} catch (e: CancellationException) {
184+
throw e
185+
} catch (e: Throwable) {
186+
waiter.completeExceptionally(
187+
ApiError("Cannot decode response for request ${raw.id}: ${e.message}")
188+
.apply { initCause(e) },
189+
)
190+
}
191+
}
192+
}
193+
194+
private fun failAllWaiters(cause: Throwable) {
195+
val error =
196+
cause as? ApiError
197+
?: ApiError("Coordinator comm closed: ${cause.message}").apply { initCause(cause) }
198+
readError = error
199+
pending.keys.forEach { id -> pending.remove(id)?.completeExceptionally(error) }
200+
}
141201
}

‎java-sdk/sdk/src/main/kotlin/org/apache/airflow/sdk/execution/Frame.kt‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,12 @@ import org.apache.airflow.sdk.execution.comm.Discriminator
2828
import org.msgpack.core.MessagePack
2929
import java.io.ByteArrayOutputStream
3030

31+
data class RawFrame(
32+
val id: Int,
33+
val rawBody: Any?,
34+
val rawError: Any?,
35+
)
36+
3137
object Frame {
3238
private val mapper =
3339
ObjectMapper().apply {
@@ -43,7 +49,7 @@ object Frame {
4349
body: Any,
4450
): ByteArray = encodeFrame(id, body)
4551

46-
fun decode(bytes: ByteArray): IncomingFrame {
52+
fun decodeRaw(bytes: ByteArray): RawFrame {
4753
val unpacker = MessagePack.newDefaultUnpacker(bytes)
4854
val headerSize = unpacker.unpackArrayHeader()
4955
check(headerSize >= 1) { "Unexpected Task SDK frame arity $headerSize" }
@@ -53,8 +59,14 @@ object Frame {
5359
val rawError = if (headerSize >= 3) unpacker.unpackAny() else null
5460
unpacker.close()
5561

56-
val body = decodeMessage(rawError) ?: decodeMessage(rawBody)
57-
return IncomingFrame(id, body)
62+
return RawFrame(id, rawBody, rawError)
63+
}
64+
65+
fun decodeBody(raw: RawFrame): Any? = decodeMessage(raw.rawError) ?: decodeMessage(raw.rawBody)
66+
67+
fun decode(bytes: ByteArray): IncomingFrame {
68+
val raw = decodeRaw(bytes)
69+
return IncomingFrame(raw.id, decodeBody(raw))
5870
}
5971

6072
fun lengthPrefix(length: Int) =

‎java-sdk/sdk/src/main/kotlin/org/apache/airflow/sdk/execution/Logger.kt‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -198,15 +198,15 @@ internal class Logger(
198198
arguments: Map<String, Any> = emptyMap(),
199199
) = log(Level.DEBUG, message, arguments)
200200

201-
fun error(
201+
fun warning(
202202
message: String,
203203
arguments: Map<String, Any> = emptyMap(),
204-
) = log(Level.ERROR, message, arguments)
204+
) = log(Level.WARNING, message, arguments)
205205

206-
fun warning(
206+
fun error(
207207
message: String,
208208
arguments: Map<String, Any> = emptyMap(),
209-
) = log(Level.WARNING, message, arguments)
209+
) = log(Level.ERROR, message, arguments)
210210

211211
private fun log(
212212
level: Level,

0 commit comments

Comments
 (0)