TATableau · Code review
Community homeHot discussions Latest posts Best of Knowledge base Resources About

Tableau · Code review · Interview notes: how I answered a question about the tableau-review concurrency model

645
TAr/tableau-review·posted by li_ming·2 hours agoTooling

Interview notes: how I answered a question about the tableau-review concurrency model

Most tableau-review articles stop at "how to use it" and never cover "when not to use it". This is an attempt at the second half.

We also fixed monitoring along the way: replaced average-based alerts with percentiles and split them per endpoint. False alerts dropped by about seventy percent and the on-call rotation visibly cheered up.

# Load test: do not jump straight to the max concurrency.
# Ramp it up, otherwise you miss the knee.
for c in 50 100 200 400 800; do
  wrk -t8 -c$c -d60s --latency http://127.0.0.1:8080/api/feed
  sleep 20
done

The first thing was to collapse the variables. We were changing config and upgrading the version at the same time, and afterwards nobody could say which change caused what. We rolled back to moving one variable at a time, re-ran three times, and only then did the curve settle. Tedious, but not skippable.

On trade-offs, my view is this: if nobody on the team owns this area long-term, do not introduce a second mechanism. With two coexistence you first have to work out which one is even in play when things break, and that costs far more than the performance you saved.

279 comments

279 comments

· first 120 loaded
M
Sslow_queryOP·2 days ago

One counter-example: below tableau-review 7.4 the semantics of that code are different, so do not copy it verbatim. We got burned in staging and rolled back once.

467
Zzhu_zong·2 days ago

I see point 3 differently. The trade-off depends on your read/write ratio: read-heavy with little writing means caching actually widens the inconsistency window.

141
Aalice_dev·2 days ago

Sharing our numbers, 8 cores 16GB, same scenario:

| Concurrency | P50 | P99 |
|---|---|---|
| 200 | 12ms | 88ms |
| 500 | 31ms | 340ms |

P99 clearly collapses at 500 concurrency, which lines up with your knee point.

518
Sslow_queryOP·2 days ago

I see point 3 differently. The trade-off depends on your read/write ratio: read-heavy with little writing means caching actually widens the inconsistency window.

274
Sslow_queryMod·2 days agoedited

There is actually a simpler fix that needs no architecture change: move this check up to the gateway and the problem disappears. The cost is one extra lookup at the gateway.

95
Wwinter·2 days ago

We have run this in production for two years without hitting it. That said, we never reached this scale, so our experience is not really evidence here.

491
Sswoole_lee·2 days ago

We have run this in production for two years without hitting it. That said, we never reached this scale, so our experience is not really evidence here.

437
Llinlin·2 days ago

Has anyone run a controlled experiment? I did, reducing it to a single variable, and the difference was 4% — within noise. So I suspect the main cause is something else.

436
Llinlin·2 days ago

Has anyone run a controlled experiment? I did, reducing it to a single variable, and the difference was 4% — within noise. So I suspect the main cause is something else.

392
Kkernel_panic·just now

Can you give a minimal reproduction? I ran it locally for ten minutes and could not reproduce on macOS with the latest version.

366
Rran_bo·12 minutes ago

Worth learning from this debugging approach. We went straight at the logs and took a much longer route.

179
Kkite·2 days ago

This is not a tableau-review problem, it is a usage problem. The docs say this API is not thread-safe and you must lock around it yourself.

1
Hhuang_keOP·2 days ago

Saved. I am reworking this area this week — this saves a lot of wrong turns.

22
Aalice_dev·12 minutes ago

This is not a tableau-review problem, it is a usage problem. The docs say this API is not thread-safe and you must lock around it yourself.

13
Wwinter·1 hour ago

This matches what we see in production. We only hit it past 3k QPS; the earlier load tests showed nothing — the test traffic was too clean, with no long-tail requests.

83
Bbob_chen·2 days ago

I just read the tableau-review source — the author actually explains the reasoning in a comment, roughly "so that it degrades into predictable behaviour in extreme cases".

60
Aalice_dev·2 days ago

This matches what we see in production. We only hit it past 3k QPS; the earlier load tests showed nothing — the test traffic was too clean, with no long-tail requests.

34
Rran_bo·2 days ago

Can you give a minimal reproduction? I ran it locally for ten minutes and could not reproduce on macOS with the latest version.

42
Mmike_xu·yesterdayedited

Has anyone run a controlled experiment? I did, reducing it to a single variable, and the difference was 4% — within noise. So I suspect the main cause is something else.

364
Zzhu_zong·2 days ago

Thanks for sharing real numbers — far more useful than the articles that only cover concepts.

215
WwinterOP·2 days ago

Saved. I am reworking this area this week — this saves a lot of wrong turns.

294
Hhuang_keMod·3 minutes ago

There is actually a simpler fix that needs no architecture change: move this check up to the gateway and the problem disappears. The cost is one extra lookup at the gateway.

319
Zzhu_zongOP·2 days ago

I see point 3 differently. The trade-off depends on your read/write ratio: read-heavy with little writing means caching actually widens the inconsistency window.

187
Kkite·2 days ago

This matches what we see in production. We only hit it past 3k QPS; the earlier load tests showed nothing — the test traffic was too clean, with no long-tail requests.

232
Kkite·2 days ago

Can you give a minimal reproduction? I ran it locally for ten minutes and could not reproduce on macOS with the latest version.

228
Bbob_chen·2 days ago

Sharing our numbers, 8 cores 16GB, same scenario:

| Concurrency | P50 | P99 |
|---|---|---|
| 200 | 12ms | 88ms |
| 500 | 31ms | 340ms |

P99 clearly collapses at 500 concurrency, which lines up with your knee point.

156
Rran_bo·2 days ago

One counter-example: below tableau-review 7.4 the semantics of that code are different, so do not copy it verbatim. We got burned in staging and rolled back once.

225
Nnikic·2 days ago

One counter-example: below tableau-review 7.4 the semantics of that code are different, so do not copy it verbatim. We got burned in staging and rolled back once.

218
Zzhou_yi·5 hours ago

One counter-example: below tableau-review 7.4 the semantics of that code are different, so do not copy it verbatim. We got burned in staging and rolled back once.

206
Wwinter·2 days ago

Agreeing with the above. One addition: with this option enabled the GC count in your metrics doubles, so adjust the alert threshold at the same time or it will keep firing.

178
Zzhou_yiMod·2 hours ago

There is actually a simpler fix that needs no architecture change: move this check up to the gateway and the problem disappears. The cost is one extra lookup at the gateway.

172
Mmike_xu·2 days ago

I just read the tableau-review source — the author actually explains the reasoning in a comment, roughly "so that it degrades into predictable behaviour in extreme cases".

166
Rran_bo·2 days ago

Can you give a minimal reproduction? I ran it locally for ten minutes and could not reproduce on macOS with the latest version.

302
Sswoole_lee·1 hour ago

Worth learning from this debugging approach. We went straight at the logs and took a much longer route.

115
Bbob_chen·2 days agoedited

This is not a tableau-review problem, it is a usage problem. The docs say this API is not thread-safe and you must lock around it yourself.

133
Zzhou_yi·5 hours ago

Sharing our numbers, 8 cores 16GB, same scenario:

| Concurrency | P50 | P99 |
|---|---|---|
| 200 | 12ms | 88ms |
| 500 | 31ms | 340ms |

P99 clearly collapses at 500 concurrency, which lines up with your knee point.

129
Cchen_dev·5 hours ago

Agreeing with the above. One addition: with this option enabled the GC count in your metrics doubles, so adjust the alert threshold at the same time or it will keep firing.

128
Oops_wang·12 minutes ago

I just read the tableau-review source — the author actually explains the reasoning in a comment, roughly "so that it degrades into predictable behaviour in extreme cases".

304
Cchen_dev·2 days ago

This is not a tableau-review problem, it is a usage problem. The docs say this API is not thread-safe and you must lock around it yourself.

452
Kkernel_panicOP·yesterday

A question: what changes in a container with a 512Mi memory limit? That is how we run it in production.

152
LlinlinOP·1 hour ago

Can you give a minimal reproduction? I ran it locally for ten minutes and could not reproduce on macOS with the latest version.

134
Lli_ming·1 hour ago

Thanks for sharing real numbers — far more useful than the articles that only cover concepts.

138
Ttang_hao·2 days agoLevel 6

Sharing our numbers, 8 cores 16GB, same scenario:

| Concurrency | P50 | P99 |
|---|---|---|
| 200 | 12ms | 88ms |
| 500 | 31ms | 340ms |

P99 clearly collapses at 500 concurrency, which lines up with your knee point.

236
Nnikic·2 days ago

Thanks for sharing real numbers — far more useful than the articles that only cover concepts.

1
WwinterOP·2 days ago

A question: what changes in a container with a 512Mi memory limit? That is how we run it in production.

102
Mmike_xu·2 days ago

Can you give a minimal reproduction? I ran it locally for ten minutes and could not reproduce on macOS with the latest version.

214
Sslow_query·2 days ago

A question: what changes in a container with a 512Mi memory limit? That is how we run it in production.

117
Rran_bo·2 days agoLevel 6

Sharing our numbers, 8 cores 16GB, same scenario:

| Concurrency | P50 | P99 |
|---|---|---|
| 200 | 12ms | 88ms |
| 500 | 31ms | 340ms |

P99 clearly collapses at 500 concurrency, which lines up with your knee point.

7
KkiteMod·2 days ago

This matches what we see in production. We only hit it past 3k QPS; the earlier load tests showed nothing — the test traffic was too clean, with no long-tail requests.

89
Lli_mingOP·2 days agoedited

I see point 3 differently. The trade-off depends on your read/write ratio: read-heavy with little writing means caching actually widens the inconsistency window.

2
Aalice_dev·2 days ago

We have run this in production for two years without hitting it. That said, we never reached this scale, so our experience is not really evidence here.

59
Sslow_queryOP·2 days ago

This is not a tableau-review problem, it is a usage problem. The docs say this API is not thread-safe and you must lock around it yourself.

51
Kkernel_panic·2 days ago

One counter-example: below tableau-review 7.4 the semantics of that code are different, so do not copy it verbatim. We got burned in staging and rolled back once.

39
Hhuang_ke·3 minutes ago

Thanks for sharing real numbers — far more useful than the articles that only cover concepts.

105
Bbob_chenOP·12 minutes ago

Saved. I am reworking this area this week — this saves a lot of wrong turns.

302
Ttang_hao·just now

Thanks for sharing real numbers — far more useful than the articles that only cover concepts.

487
Lli_ming·2 days ago

There is actually a simpler fix that needs no architecture change: move this check up to the gateway and the problem disappears. The cost is one extra lookup at the gateway.

118
Ddev_zhou·2 days ago

One counter-example: below tableau-review 7.4 the semantics of that code are different, so do not copy it verbatim. We got burned in staging and rolled back once.

470
Rrase·2 days ago

I just read the tableau-review source — the author actually explains the reasoning in a comment, roughly "so that it degrades into predictable behaviour in extreme cases".

276
Mmike_xu·2 days agoedited

Saved. I am reworking this area this week — this saves a lot of wrong turns.

77
Hhuang_keOP·just nowLevel 6

We have run this in production for two years without hitting it. That said, we never reached this scale, so our experience is not really evidence here.

43
Wwinter·2 days ago

I see point 3 differently. The trade-off depends on your read/write ratio: read-heavy with little writing means caching actually widens the inconsistency window.

42
Aalice_dev·2 days agoLevel 6

Worth learning from this debugging approach. We went straight at the logs and took a much longer route.

29
Zzhu_zong·2 days agoLevel 6

This is not a tableau-review problem, it is a usage problem. The docs say this API is not thread-safe and you must lock around it yourself.

5
Wwinter·28 minutes agoedited

Agreeing with the above. One addition: with this option enabled the GC count in your metrics doubles, so adjust the alert threshold at the same time or it will keep firing.

45
Sslow_query·2 days ago

I see point 3 differently. The trade-off depends on your read/write ratio: read-heavy with little writing means caching actually widens the inconsistency window.

149
Wwinter·2 days ago

Saved. I am reworking this area this week — this saves a lot of wrong turns.

124
Ddev_zhouMod·2 days ago

There is actually a simpler fix that needs no architecture change: move this check up to the gateway and the problem disappears. The cost is one extra lookup at the gateway.

1
Rran_bo·2 days agoLevel 6

Sharing our numbers, 8 cores 16GB, same scenario:

| Concurrency | P50 | P99 |
|---|---|---|
| 200 | 12ms | 88ms |
| 500 | 31ms | 340ms |

P99 clearly collapses at 500 concurrency, which lines up with your knee point.

42
Wwinter·2 days ago

Worth learning from this debugging approach. We went straight at the logs and took a much longer route.

5
Zzhou_yi·2 days ago

Worth learning from this debugging approach. We went straight at the logs and took a much longer route.

10
LlinlinOP·just now

Can you give a minimal reproduction? I ran it locally for ten minutes and could not reproduce on macOS with the latest version.

189
Oops_wang·2 days ago

This matches what we see in production. We only hit it past 3k QPS; the earlier load tests showed nothing — the test traffic was too clean, with no long-tail requests.

39
Lli_ming·2 days agoedited

Has anyone run a controlled experiment? I did, reducing it to a single variable, and the difference was 4% — within noise. So I suspect the main cause is something else.

2
Zzhou_yiOP·2 days ago

There is actually a simpler fix that needs no architecture change: move this check up to the gateway and the problem disappears. The cost is one extra lookup at the gateway.

34
Bbob_chen·2 days ago

Agreeing with the above. One addition: with this option enabled the GC count in your metrics doubles, so adjust the alert threshold at the same time or it will keep firing.

349
Zzhou_yi·28 minutes ago

Thanks for sharing real numbers — far more useful than the articles that only cover concepts.

469
Kkite·2 days ago

A question: what changes in a container with a 512Mi memory limit? That is how we run it in production.

1
Bbob_chenMod·12 minutes ago

Has anyone run a controlled experiment? I did, reducing it to a single variable, and the difference was 4% — within noise. So I suspect the main cause is something else.

93
Wwinter·12 minutes ago

Saved. I am reworking this area this week — this saves a lot of wrong turns.

106
Ttang_hao·2 days ago

I see point 3 differently. The trade-off depends on your read/write ratio: read-heavy with little writing means caching actually widens the inconsistency window.

217
Hhuang_ke·2 days ago

We have run this in production for two years without hitting it. That said, we never reached this scale, so our experience is not really evidence here.

187
Wwinter·2 days ago

A question: what changes in a container with a 512Mi memory limit? That is how we run it in production.

24
Nnikic·2 days agoedited

Agreeing with the above. One addition: with this option enabled the GC count in your metrics doubles, so adjust the alert threshold at the same time or it will keep firing.

84
Wwinter·2 hours ago

Can you give a minimal reproduction? I ran it locally for ten minutes and could not reproduce on macOS with the latest version.

80
Rran_bo·28 minutes ago

Worth learning from this debugging approach. We went straight at the logs and took a much longer route.

79
Mmike_xuMod·yesterday

Agreeing with the above. One addition: with this option enabled the GC count in your metrics doubles, so adjust the alert threshold at the same time or it will keep firing.

342
Sslow_query·2 days ago

Agreeing with the above. One addition: with this option enabled the GC count in your metrics doubles, so adjust the alert threshold at the same time or it will keep firing.

117
Zzhu_zong·2 days ago

Agreeing with the above. One addition: with this option enabled the GC count in your metrics doubles, so adjust the alert threshold at the same time or it will keep firing.

142
Ttang_hao·3 minutes agoedited

We have run this in production for two years without hitting it. That said, we never reached this scale, so our experience is not really evidence here.

1
Sslow_query·2 days agoedited

There is actually a simpler fix that needs no architecture change: move this check up to the gateway and the problem disappears. The cost is one extra lookup at the gateway.

411
Aalice_dev·2 days ago

Sharing our numbers, 8 cores 16GB, same scenario:

| Concurrency | P50 | P99 |
|---|---|---|
| 200 | 12ms | 88ms |
| 500 | 31ms | 340ms |

P99 clearly collapses at 500 concurrency, which lines up with your knee point.

356
Zzhu_zong·2 days ago

Thanks for sharing real numbers — far more useful than the articles that only cover concepts.

1
Kkite·2 days agoedited

I just read the tableau-review source — the author actually explains the reasoning in a comment, roughly "so that it degrades into predictable behaviour in extreme cases".

76
Llinlin·1 hour ago

I just read the tableau-review source — the author actually explains the reasoning in a comment, roughly "so that it degrades into predictable behaviour in extreme cases".

69
Rran_bo·2 days ago

This matches what we see in production. We only hit it past 3k QPS; the earlier load tests showed nothing — the test traffic was too clean, with no long-tail requests.

56
Aalice_devOP·2 days ago

Thanks for sharing real numbers — far more useful than the articles that only cover concepts.

227
Kkite·2 days ago

Has anyone run a controlled experiment? I did, reducing it to a single variable, and the difference was 4% — within noise. So I suspect the main cause is something else.

56
Rran_bo·2 days ago

Has anyone run a controlled experiment? I did, reducing it to a single variable, and the difference was 4% — within noise. So I suspect the main cause is something else.

43
Sslow_query·2 days ago

A question: what changes in a container with a 512Mi memory limit? That is how we run it in production.

3
Zzhou_yi·2 days ago

This is not a tableau-review problem, it is a usage problem. The docs say this API is not thread-safe and you must lock around it yourself.

25
Llinlin·2 days ago

Saved. I am reworking this area this week — this saves a lot of wrong turns.

327
Kkernel_panic·2 days ago

A question: what changes in a container with a 512Mi memory limit? That is how we run it in production.

24
Nnikic·2 days ago

One counter-example: below tableau-review 7.4 the semantics of that code are different, so do not copy it verbatim. We got burned in staging and rolled back once.

20
Cchen_devOP·2 days ago

Worth learning from this debugging approach. We went straight at the logs and took a much longer route.

62
Kkernel_panic·1 hour ago

This matches what we see in production. We only hit it past 3k QPS; the earlier load tests showed nothing — the test traffic was too clean, with no long-tail requests.

16
Mmike_xu·2 days ago

This is not a tableau-review problem, it is a usage problem. The docs say this API is not thread-safe and you must lock around it yourself.

128
Sswoole_lee·2 days ago

A question: what changes in a container with a 512Mi memory limit? That is how we run it in production.

8
Rrase·12 minutes ago

There is actually a simpler fix that needs no architecture change: move this check up to the gateway and the problem disappears. The cost is one extra lookup at the gateway.

7
Cchen_dev·2 days ago

I just read the tableau-review source — the author actually explains the reasoning in a comment, roughly "so that it degrades into predictable behaviour in extreme cases".

6
Hhuang_keMod·2 days ago

This matches what we see in production. We only hit it past 3k QPS; the earlier load tests showed nothing — the test traffic was too clean, with no long-tail requests.

6
Ddev_zhou·28 minutes ago

Sharing our numbers, 8 cores 16GB, same scenario:

| Concurrency | P50 | P99 |
|---|---|---|
| 200 | 12ms | 88ms |
| 500 | 31ms | 340ms |

P99 clearly collapses at 500 concurrency, which lines up with your knee point.

5
Bbob_chen·2 days ago

I see point 3 differently. The trade-off depends on your read/write ratio: read-heavy with little writing means caching actually widens the inconsistency window.

1
Rran_bo·28 minutes ago

Worth learning from this debugging approach. We went straight at the logs and took a much longer route.

286
Zzhu_zong·2 days ago

We have run this in production for two years without hitting it. That said, we never reached this scale, so our experience is not really evidence here.

51
Sswoole_lee·2 days ago

Has anyone run a controlled experiment? I did, reducing it to a single variable, and the difference was 4% — within noise. So I suspect the main cause is something else.

390
Cchen_dev·2 days ago

We have run this in production for two years without hitting it. That said, we never reached this scale, so our experience is not really evidence here.

55
Wwinter·2 hours ago

One counter-example: below tableau-review 7.4 the semantics of that code are different, so do not copy it verbatim. We got burned in staging and rolled back once.

8
Bbob_chen·2 days ago

I just read the tableau-review source — the author actually explains the reasoning in a comment, roughly "so that it degrades into predictable behaviour in extreme cases".

40
Kkernel_panic·2 days ago

Saved. I am reworking this area this week — this saves a lot of wrong turns.

1

This is the post detail page /en/c/tableau-review/post/p4. Posts and comments are generated deterministically from a seeded PRNG, so the same post always renders the same content and the link can be shared, reloaded and indexed. In production this page reads MySQL for the post, Redis for hot-post caching, and fetches the whole comment tree in a single query on the path column.

See the database schema →