Do not send SKIP LOCKED to servers that do not implement it - #71250
Do not send SKIP LOCKED to servers that do not implement it#712501fanwang wants to merge 1 commit into
Conversation
|
Oh, fascinating Stefan, I am not familiar with TiDB. |
bda3af6 to
42455fd
Compare
Hey Vikram, I was actually just reading more about #46175 and #65453. To be forthcoming - I'm not proposing official support for Airflow on a TiDB backend, and this PR isn't meant to be a step toward asking for it, at least for now. Let me share
For scale context, our largest single cluster is reaching 25k+ Dags and still growing. Most of the scaling problems we've hit have answers that stay close to upstream: add schedulers, tune the executor, etc.. The metadata DB is the one that doesn't really scale other than vertically, *can't easily be sharded, since the scheduler critical section, TI state, XCom and event logs all write to one primary, and we always just get a bigger box (we've done that many times) or changes that drift away from OSS Airflow. I'd rather not drift. We've also been trying read/write splitting to take some pressure off from it, since we already run read replicas. Either route reads explicitly in Airflow's own source, or put query routing rules in a ProxySQL layer so Airflow core can stay generic. No numbers to share yet, but the challenge is clear - it only moves read load, and the scheduler's hot path is writes/txns. That makes distributing writes interesting, and TiDB uses the MySQL wire protocol. I haven't reached to the point to benchmarked it yet, so "scales writes" is a motivation and not a result I can show at this point. So far this is just a small local cluster with Airflow pointed at it, checking the SQL queries. I do plan on trying this on our internal Airflow and TiDB clusters. Happy to share what we find running Airflow on TiDB at scale if folks are interested. |
The scheduler claims task instances with SELECT ... FOR UPDATE SKIP LOCKED and relies on the clause to keep concurrent schedulers off the same rows. A server that accepts the clause and discards it hands the same rows to every scheduler at once, with no error and no warning, so the safety property is lost silently rather than loudly. Signed-off-by: 1fanwang <1fannnw@gmail.com>
42455fd to
d12c1fe
Compare
|
I don’t quite understand. So TiDB silently ignores SKIP LOCKED, but this PR simply makes Airflow not send that. So the end result is unchanged? Why is this PR needed? |
Closing the PR as not applicable — sorry for the noise @uranusjr TP you are right that this changes nothing, adds no value on making Airflow run on TIDB today. Testing Airflow on TiDB further showed it can't even keep a scheduler loop alive, I think maybe due to a TiDB planner bug pingcap/tidb#67715, will have to look into this further and test, closing for now |
Airflow claims work with
SELECT ... FOR UPDATE ... SKIP LOCKEDin eleven places. On TiDB, any ofthose that also joins another table fails with error 1105 and takes down
SchedulerJob._run_scheduler_loop. Nothing is scheduled until it restarts. One scheduler is enoughto hit it.
LogicalLock.PruneColumnsprotects each locked table's handle column from pruning, but only for supported lock types.
SKIP LOCKEDis missing fromisSelectForUpdateLockType,so the handle is pruned while
TblID2Handlestill references it. Filed aspingcap/tidb#67715. Without a join there is no
second handle to lose, so the clause is silently ignored instead.
with_row_locks()already degrades for MySQL-family servers that cannot lock. This extends it toservers that accept
SKIP LOCKEDwithout implementing it: read the banner once per engine, fallback to plain
FOR UPDATE. If the probe fails, previous behaviour is kept. PostgreSQL and MySQLare untouched.
Testing Done
An api-server and two
airflow schedulerprocesses against one TiDB v8.5.1. Six producer Dag runsfanning out to six asset-scheduled consumers. Same deployment, once per branch:
queuedx6{None: 36}success{success: 66}Raw logs
without:
with:
_executable_task_instances_to_queuedis unaffected: it runs inside theslot_pool ... FOR UPDATE NOWAITcritical section, which TiDB implements correctly. Banner matching is the weakest availablefix — a capability probe in
airflow db check, or docs stating the contract, would generalisebetter. Happy to convert.
Was generative AI tooling used to co-author this PR?
Generated-by: GitHub Copilot CLI (Claude Opus 5) following the guidelines