[All] Correct SDK documentation and examples - #729
Conversation
teodordelibasic-db
changed the title
Signed-off-by: teodordelibasic-db <teodor.delibasic@databricks.com>
0e03af6 to
9a004c1
Compare
| # ack = stream.ingest_record(record_dict) # Deprecated | ||
| # offset = ack.wait_for_ack() # Extra step needed | ||
|
|
||
| end_time = time.time() |
There was a problem hiding this comment.
ingest_record_nowait() and ingest_records_nowait() spawn detached tasks and discard enqueue errors. flush() can complete before those tasks allocate offsets, so this example may report durability while losing submissions.
@teodordelibasic-db @elenagaljak-db I think we should remove nowait from examples and put it on a deprecation path.
| unacked = stream.get_unacked_records() # Returns List[bytes] | ||
| stream.close() | ||
| except ZerobusException as e: | ||
| unacked = list(stream.get_unacked_records()) |
There was a problem hiding this comment.
This catch also handles immediate enqueue errors that leave the stream active. Calling get_unacked_records() or recreate_stream() then fails and masks the original error.
Please separate enqueue failures from terminal failures, close before recovery, and protect the replacement stream with try/finally.
| when records are acknowledged by the server or encounter errors. | ||
| Subclass this in Python to create custom callbacks that are invoked once per | ||
| logical ingest submission. A batch submission produces one callback, not one | ||
| callback per record in the batch. |
There was a problem hiding this comment.
Pre-queue validation, size, type, and closed-stream failures do not generate callbacks, so this does not apply to every attempted submission.
Please qualify it as one callback per successfully queued submission that later acknowledges or fails.
| ```python | ||
| unacked_batches = stream.get_unacked_batches() # Returns List[List[bytes]] |
There was a problem hiding this comment.
This ranks per-record ingest_record_nowait() highest but omits the batch APIs. Batch ingestion amortizes the Python→Rust crossing and is the preferred hot path. Additionally, detached nowait submissions are not safely synchronized with flush.
Please include the batch APIs and recommend ingest_records_offset() plus one flush for reliable bulk ingestion.
| * ); | ||
| * ``` | ||
| * | ||
| * **How to use custom authentication (PAT, etc.):** |
There was a problem hiding this comment.
The exported interface requires async getHeaders(): Promise<...>, while this example and createStream() require synchronous getHeadersCallback. A class implementing the public interface cannot be passed to the documented API.
Please expose and document one provider type matching createStream().
| /// | ||
| /// * `stream` - The failed or closed stream to recreate | ||
| /// * `stream` - The terminally failed stream to recreate. The TypeScript wrapper | ||
| /// must not have been closed because `close()` releases its native handle. |
There was a problem hiding this comment.
LLM find:
TypeScript custom credentials cannot refresh
Affected: typescript/src/lib.rs:1024-1046
The callback runs once during stream creation and its result is stored in StaticHeadersProvider. Long-running streams therefore reuse the original token during recovery, so rotating or expiring custom credentials eventually break reconnection.
Keep the threadsafe callback in a HeadersProvider adapter and invoke it whenever the Rust core requests fresh headers. Support invalidation if required by the core contract.
| // Optional: Inspect what needs recovery (must be called on closed stream) | ||
| // Optional: Inspect what needs recovery after a terminal stream failure. | ||
| const unackedBatches = await stream.getUnackedBatches(); | ||
| console.log(`Batches to recover: ${unackedBatches.length}`); |
There was a problem hiding this comment.
getUnackedBatches() runs before cleanup protection, and this catch can also handle non-terminal failures. An inspection failure can leak the original stream and mask the ingestion error.
Please use an outer finally, recover only from confirmed terminal failures, and close every replacement stream in a nested finally.
| AirQuality.create({ deviceName: 'sensor-002', temp: 23, humidity: 67 }), | ||
| AirQuality.create({ deviceName: 'sensor-003', temp: 24, humidity: 69 }) |
There was a problem hiding this comment.
typescript/examples/proto/batch.ts:113-116
Waiting here before queueing the next batch serializes the example into one server round trip per batch.
Please queue all demonstration batches first, then call flush() once or wait only on the final offset. The JSON batch example has the same issue.
|
|
||
| ### Code Highlights | ||
|
|
||
| **Offset-based API (Recommended):** |
There was a problem hiding this comment.
typescript/examples/json/README.md:65-71
This first “Recommended” pattern immediately waits after one ingest. Although valid for strict low-volume confirmation, it is not the default pattern readers should copy.
Please show loop-then-flush() first and move this into a clearly labeled low-volume section. The Protobuf README has the same ordering issue.
|
|
||
| main().catch((error) => { | ||
| console.error('Fatal error:', error); | ||
| }); |
There was a problem hiding this comment.
This logs a fatal error but leaves the process exit status as zero, so copied CLI or CI code can report success after ingestion fails.
Please set process.exitCode = 1 or rethrow.
|
I don't see any changes to GO SDK while it was mentioned in PR desc. Maybe you audited it and there were no stale docs/examples? |
| >>> | ||
| >>> # New optimized API | ||
| >>> offset = stream.ingest_record_offset(b"data") | ||
| >>> offset = stream.ingest_record_offset('{"value": "data"}') |
There was a problem hiding this comment.
python/zerobus/init.py:13
Callbacks represent logical submissions and can correspond to batches, so “Record acknowledged” is too specific.
Please use “Submission acknowledged” for consistency.
|
LLM instrumented pass from my side about gaps not covered by this PR, includes also high severity bugs in SDK: Consumer installation incorrectly requires Rust and go generate make build-go is not Go-only Go user-agent version is stale RecordAck.Await() is documented as immediate Go concurrency guidance contradicts the API Go copyable snippets omit durability barriers Pure-Go recovery treats every flush error as terminal Pure-Go batch callback semantics are wrong P1 — Java Callback exceptions are not contained as documented flush() is presented as a callback-drain barrier Source-build/example commands produce non-runnable artifacts macOS is advertised but not released Java 8 source-build requirement is false Proto examples depend on a missing generated file GenerateProto overclaims STRUCT support GenerateProto commands use stale artifact names/version Configuration defaults are stale Negative recovery backoff validation is documented but absent Several copyable stream snippets can leak JNI handles Primary README omits Java thread-safety constraints P1 — Python and TypeScript Python exposes fabricated stream IDs and states TypeScript TableProperties docs select the wrong format TypeScript inflight default is wrong by 100× TypeScript claims GC eliminates cleanup P1 — Rust, C++, .NET and C FFI Primary Rustdoc still teaches per-ingest waits Rust docs reference removed APIs C++ recovery fails after a flush timeout .NET recovery doc has the same active-stream error .NET JSON example can report partial ingestion as success First .NET API examples omit durability confirmation C FFI README omits the required lifecycle P2 — smaller stale items |
2 participants
What changes are proposed in this pull request?
Correct README snippets, checked-in examples, and public API doc examples across Rust, Python, Java, Go, pure Go, TypeScript, C++, and .NET so they compile and demonstrate the current APIs. The changes fix API names and arguments, stream format selection, dependency setup, recovery and callback semantics, resource cleanup, and queue-then-flush ingestion. They also add the generated-message fixture needed to run the .NET Protobuf example.
The documentation had drifted as the public APIs evolved, leaving examples that either failed to compile or demonstrated incorrect recovery, durability, and throughput patterns. Keeping these examples aligned with the supported APIs prevents users from copying invalid or unnecessarily slow client code.
How is this tested?
All runnable examples and executable documentation snippets were compiled or type-checked. Representative JSON, Protobuf, Arrow, batch, recovery, and custom-header paths were also exercised end to end.
The affected SDK formatters, linters, unit tests, example builds, and doc tests pass. The .NET suite passes 47 unit and 54 integration tests. TypeScript build, tests, and example type-check pass; its all-features Clippy check still reports three pre-existing warnings unrelated to these documentation changes. The C++ Arrow example was source-reviewed but not linked because Apache Arrow C++ was unavailable; the other C++ examples built and their JSON batch and custom-header paths were exercised.