[Kafka] Raise the producer message cap so policy-max transactions publish - #330
[Kafka] Raise the producer message cap so policy-max transactions publish#330Calgooon wants to merge 2 commits into
Conversation
…lish Producer.MaxMessageBytes was never set, so sarama's 1 MiB default applied. TopicPropagation carries raw_tx base64-encoded inside JSON (x4/3), so transactions above ~750 KB of EF failed at producer.Send with "failed to submit" while GET /policy advertised 10 MiB. Add kafka.max_message_bytes (default 16 MiB) for both producers, provision the broker and per-topic caps to match in compose and deploy, pin the default in a test, and document the three limits that must agree. Fixes bsv-blockchain#329 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Calhoun <jtcalhou@umich.edu>
👋 Thanks, @Calgooon!This pull request comes from a fork. For security, our CI runs in a restricted mode.
Thanks for contributing to bsv-blockchain/arcade! 🚀 |
galt-tr
left a comment
There was a problem hiding this comment.
Issue #329: valid. On c49c8dd neither producer sets Producer.MaxMessageBytes, so sarama v1.60.1's 1 MiB default applies (config.go:569); the check runs in the async dispatcher on the uncompressed ByteSize (async_producer.go:653-655) and SyncProducer wraps that producer, so the base64 raw_tx in the propagation envelope (services/api_server/handlers.go:952-958) trips it at ~750 KB of tx and maps to 500 {"error":"failed to submit"} (handlers.go:967-971). services/bump_builder/builder.go:410-415 already chunks bulk MINED events around the same default.
Overall the change is small and correctly scoped: both producers get the cap, NewBroker threads cfg.MaxMessageBytes through, NewSaramaBroker/WithLogger keep their signatures, and the compose and dev-StatefulSet broker/topic sides move in the same PR. gofmt, go build ./..., go vet (incl. -tags=e2e ./tests/e2e/), go test ./kafka/... ./config/... and golangci-lint are clean locally (CI's Test Suite job does not run for fork PRs). Two things need fixing before merge: the new key has no viper.SetDefault, so ARCADE_KAFKA_MAX_MESSAGE_BYTES is silently ignored (the only way deploy/ configures kafka); and the 16 MiB default does not cover a policy-max transaction once input_txids and EF per-input data are counted (measured ~21 MiB for a 9.99 MiB P2PKH consolidation submitted as EF), so the const comment, test and docs overstate what the default guarantees. Details inline.
Not in the diff:
services/bump_builder/builder.go:410-415and:458-462still cite "the 1 MiB default Producer.MaxMessageBytes" as the reason formaxTxIDsPerBulkEvent = 5000. The chunking should stay (an operator can setkafka.max_message_bytesbelow the new default), but the comments should point atkafka.max_message_bytes/kafka.DefaultProducerMaxMessageBytesinstead of a default that no longer applies.
Checked and not an issue: the consumer can read 16 MiB records (newSaramaConsumerConfig leaves Consumer.Fetch.Max at 0, so the ErrMessageTooLarge branch at consumer.go:917-919 is unreachable and the fetch size grows to the partial batch size; Fetch.MaxBytes defaults to 50 MiB); the async producer's unset Version resolves to sarama's DefaultVersion (2.8.0), so both producers size messages with record-v2 overhead; per-partition batches now roll over at 16 MiB instead of 1 MiB (produce_set.go:327-328), which matches the broker/topic caps set here and does not add memory since queued messages are already resident; rpk topic create -c/--topic-config, rpk topic alter-config --set, rpk cluster config set kafka_batch_max_bytes and the bitnami KAFKA_CFG_* mappings match the documented CLI/env forms (not executed here).
| // MaxMessageBytes caps the size of one produced Kafka message, in bytes, | ||
| // for the sarama backend (Producer.MaxMessageBytes). 0 selects the kafka | ||
| // package default (16 MiB), sized so a policy-max 10 MiB transaction still | ||
| // fits after base64 JSON encoding on TopicPropagation. The broker and the | ||
| // arcade.* topics must accept at least the same size (message.max.bytes / | ||
| // Redpanda kafka_batch_max_bytes, topic max.message.bytes); see | ||
| // docs/production-kafka.md. | ||
| MaxMessageBytes int `mapstructure:"max_message_bytes"` |
There was a problem hiding this comment.
Important kafka.max_message_bytes has no viper.SetDefault in setDefaults(), while every other kafka.* key does (L1108-1113). Load uses AutomaticEnv + viper.Unmarshal, and Unmarshal only visits keys viper already knows from defaults/config/flags, so ARCADE_KAFKA_MAX_MESSAGE_BYTES is silently dropped whenever the key is absent from the YAML. Verified against viper v1.21.0 with this exact prefix/replacer setup: viper.GetInt("kafka.max_message_bytes") returns the env value but the unmarshalled field stays 0, and adding the default fixes it. deploy/*.yaml configure kafka exclusively through ARCADE_KAFKA_* env, so as written the cap cannot be changed there at all, and an operator who raises it will keep getting the same 500 failed to submit (the comment at L1300-1301 documents this requirement for the validator keys).
viper.SetDefault("kafka.send_timeout_ms", 2000)
viper.SetDefault("kafka.max_message_bytes", 0)| // DefaultProducerMaxMessageBytes is the producer-side message cap applied when | ||
| // kafka.max_message_bytes is unset. Sarama's own default (1 MiB) is below what | ||
| // a single TopicPropagation message can legitimately be: raw_tx is JSON-encoded | ||
| // as base64 (x4/3) and the tx-size policy allows 10 MiB, so a policy-valid | ||
| // submit can need ~14 MiB. 16 MiB leaves headroom for the envelope. The | ||
| // broker must accept the same size (Kafka message.max.bytes / Redpanda | ||
| // kafka_batch_max_bytes, and the topic's max.message.bytes) or produces still | ||
| // fail broker-side; see docs/production-kafka.md. | ||
| const DefaultProducerMaxMessageBytes = 16 << 20 |
There was a problem hiding this comment.
Important The x4/3 derivation counts only raw_tx. The propagation message also carries input_txids, one 64-hex string per input with no dedup (~67 B each, services/api_server/handlers.go:37-49,952-956), and raw_tx is whatever bytes the client sent, so an EF submit adds ~34 B per P2PKH input before base64. The size policy is enforced on the non-EF serialisation (tx.Size() via gobdk SetMaxTxSizePolicy), so a consolidation-shaped tx is policy-valid well past this cap. Measured with go-sdk v1.3.3 (P2PKH inputs, one output, the same map the handler marshals):
| inputs | tx.Size() |
message, raw submit | message, EF submit |
|---|---|---|---|
| 55,000 | 7.76 MiB | 13.86 MiB | 16.24 MiB |
| 60,000 | 8.47 MiB | 15.13 MiB | 17.72 MiB |
| 70,800 | 9.99 MiB | 17.85 MiB | 20.91 MiB |
With this default an EF consolidation above ~7.5 MiB, or a raw one above ~9 MiB, still fails with the deterministic 500 the issue describes, while this comment, the new test and docs/production-kafka.md say it is covered. Either raise the default (32 MiB, matching maxSingleTxBytes, clears the policy-max P2PKH case with margin; compose/topic-init.sh and deploy/kafka.yaml need the same value) or state the real bound here: base64 of the submitted bytes plus ~67 B per input. The bound is O(inputs), so the doc should say which tx shapes the chosen default covers rather than "policy-max".
| // explicit producer message-size cap (bytes) for both the sync and async | ||
| // producers. maxMessageBytes <= 0 selects DefaultProducerMaxMessageBytes. | ||
| func NewSaramaBrokerWithMaxMessageBytes(brokers []string, consumerGroup string, logger *zap.Logger, maxMessageBytes int) (Broker, error) { | ||
| maxMessageBytes = producerMaxMessageBytes(maxMessageBytes) |
There was a problem hiding this comment.
Minor Nothing rejects a configured value at or above sarama.MaxRequestSize (100 MiB). sarama's Config.Validate only logs Producer.MaxMessageBytes must be smaller than MaxRequestSize; it will be ignored (config.go:639-640) and the dispatcher keeps using the value, so the mistake surfaces per message at produce time (wouldOverflow on MaxRequestSize-10KiB) instead of at startup. Fail fast here:
if maxMessageBytes >= int(sarama.MaxRequestSize) {
return nil, fmt.Errorf("kafka.max_message_bytes %d must be below sarama.MaxRequestSize (%d)", maxMessageBytes, sarama.MaxRequestSize)
}| // 10 MiB tx-size policy, base64-encoded inside the JSON envelope. | ||
| if minNeeded := 10485760 * 4 / 3; DefaultProducerMaxMessageBytes <= minNeeded { | ||
| t.Fatalf("DefaultProducerMaxMessageBytes %d does not clear a base64-encoded 10 MiB tx (%d)", DefaultProducerMaxMessageBytes, minNeeded) | ||
| } | ||
| if got := newSyncProducerConfig(4 << 20).Producer.MaxMessageBytes; got != 4<<20 { | ||
| t.Fatalf("explicit MaxMessageBytes = %d, want %d", got, 4<<20) | ||
| } |
There was a problem hiding this comment.
Minor minNeeded models only base64 of a non-EF 10 MiB tx: 10485760*4/3 is 13,981,013 B, which is 13.3 MiB (14.0 MB), so the "~14 MiB" wording here, in the const doc and in the docs mixes units. It omits the input_txids array and EF per-input data, which is what pushes a policy-max consolidation to ~21 MiB (see the comment on DefaultProducerMaxMessageBytes), so this assertion passes while the guarantee it describes does not hold. If the default is meant to guarantee "policy-valid submit fits", pin that with a synthetic tx (N P2PKH inputs via go-sdk with tx.Size() just under the policy, marshal the same map[string]interface{} the handler builds, assert len(b) < DefaultProducerMaxMessageBytes) so the test moves when the envelope does. The explicit-value case should also assert Validate() passes, as TestNewSyncProducerConfig_OrderingInvariants does for the default.
|
|
||
| # Message size: a policy-max 10 MiB tx is base64 (x4/3) inside the propagation | ||
| # envelope, so the broker default (1 MiB) rejects large submits at produce | ||
| # time. Keep in step with kafka.max_message_bytes (arcade-config.yaml) and |
There was a problem hiding this comment.
Minor compose/arcade-config.yaml has no kafka.max_message_bytes (its kafka: block only sets brokers), so this pointer sends the reader to a key that is not there; the documented key is in config.example.yaml. Because the compose arcade instance therefore always runs the code default, an operator who overrides MAX_MESSAGE_BYTES here changes the broker and topic caps but not the producer. Either add max_message_bytes: 16777216 to compose/arcade-config.yaml and keep this reference, or point it at config.example.yaml / kafka.DefaultProducerMaxMessageBytes.
| `TopicPropagation` carries the raw transaction base64-encoded inside a JSON | ||
| envelope, so a message is about 4/3 of the transaction size plus the envelope. | ||
| With the default 10 MiB `max_tx_size_policy` a valid submit can need ~14 MiB. |
There was a problem hiding this comment.
Minor "~14 MiB" is the base64 of a non-EF 10 MiB tx only (13.3 MiB; 14 is the MB figure). The envelope also carries input_txids (~67 B per input, not deduplicated) and an EF submit adds per-input source data, so a policy-max P2PKH consolidation submitted as EF measures ~21 MiB, above the 16 MiB the table below presents as sufficient. State the real bound (base64 of the submitted bytes + ~67 B x inputs) and which tx shapes the default covers, or raise the default together with the two broker values.
What Changed
kafka/sarama_broker.go: newDefaultProducerMaxMessageBytes(16 MiB) andNewSaramaBrokerWithMaxMessageBytes;newSyncProducerConfigtakes the cap and setsProducer.MaxMessageByteson both the sync and async producers.NewSaramaBroker/NewSaramaBrokerWithLoggerkeep their signatures and use the default.kafka/factory.go: passescfg.MaxMessageBytesthrough.config/config.go+config.example.yaml: newkafka.max_message_bytes(0 = default).compose/topic-init.sh: setskafka_batch_max_bytesand per-topicmax.message.bytes(create and idempotent alter for pre-existing topics), env-overridable viaMAX_MESSAGE_BYTES.deploy/kafka.yaml:KAFKA_CFG_MESSAGE_MAX_BYTESandKAFKA_CFG_REPLICA_FETCH_MAX_BYTES= 16 MiB.docs/production-kafka.md: "Message size" section listing the three limits that must agree; notes thebsva-infra-fluxtopic config.kafka/sarama_broker_test.go:TestSyncProducerConfig_MaxMessageBytespins the default (and that it clears a base64-encoded 10 MiB tx) and the passthrough; existing callers ofnewSyncProducerConfigupdated.Why It Was Necessary
The producers ran on sarama's default 1 MiB cap. A TopicPropagation message carries
raw_txbase64-encoded inside JSON (x4/3), so transactions above ~750 KB of EF failed atproducer.Sendwith500 failed to submiteven though every advertised policy limit (10 MiB tx, network-tracked script size) accepted them.bump_builderalready works around the same default by chunking; a single transaction cannot be chunked. Fixes #329.Testing Performed
gofmt -l kafka/ config/clean;go build ./kafka/ ./config/,go vet ./kafka/ ./config/,go test ./kafka/pass (includes the new test).make lint(golangci-lint not installed on this machine) and the compose stack;topic-init.shchanges follow the existingrpkusage in the script and are idempotent.Impact / Risk
bsva-infra-fluxchange production needs. Without the broker side, behaviour is unchanged (produce still fails broker-side with the same 500).NewSaramaBrokercallers (e2e tests) are unaffected.Notifications