feat(queue/sql): add MySQL schema and configuration#20
Conversation
behinddwalls
changed the base branch from
chore-claude-add-claude
to
sql-queue-rfc
behinddwalls
force-pushed
the
sql-queue-schema-config
branch
from
3477145 to
f312df5
Compare
behinddwalls
force-pushed
the
sql-queue-rfc
branch
from
7f9dd0c to
3506dc0
Compare
behinddwalls
force-pushed
the
sql-queue-schema-config
branch
from
f312df5 to
38c7cf2
Compare
behinddwalls
force-pushed
the
sql-queue-rfc
branch
from
3506dc0 to
5b4bd68
Compare
behinddwalls
force-pushed
the
sql-queue-schema-config
branch
from
38c7cf2 to
73e753a
Compare
behinddwalls
force-pushed
the
sql-queue-rfc
branch
from
5b4bd68 to
9b3e117
Compare
behinddwalls
force-pushed
the
sql-queue-schema-config
branch
2 times, most recently
from
825992e to
e8b1a30
Compare
behinddwalls
force-pushed
the
sql-queue-rfc
branch
from
9b3e117 to
826f9f0
Compare
behinddwalls
force-pushed
the
sql-queue-schema-config
branch
2 times, most recently
from
c4c3fe6 to
454d92d
Compare
behinddwalls
force-pushed
the
sql-queue-schema-config
branch
from
454d92d to
7dcf5fb
Compare
|
Have we tried an existing oss solution (queue over mysql) first? |
|
|
||
| -- Indexes optimized for partition-based queries with visibility | ||
| INDEX idx_topic_partition_visible_offset (topic, partition_key, invisible_until, offset), | ||
| UNIQUE KEY idx_topic_partition_id (topic, partition_key, id) |
There was a problem hiding this comment.
Should this be primary key instead?
There was a problem hiding this comment.
offset is the primary which is easier to reason it seems as per AI, primarily how it auto-increments and avoids page fragmentation, so having this secondary index is not too bad for this table.
| err := tt.config.Validate() | ||
| if tt.expectError { | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), tt.errorMsg) |
There was a problem hiding this comment.
AI likes to generate string comparisons for error instead of error types (because typically there are no error types); I think this is a bad practice that makes tests fragile.
Just asserting the error is usually ok; when required by code flow, can dedicate an error type and use Is/As semantics.
There was a problem hiding this comment.
i think it sort of validate which error is being returned and not short-circuiting due to any other errors in the call chain
There was a problem hiding this comment.
If error message is not part of the protocol, this makes test fragile to changes for not reason dictated by a change.
If it is part of the protocol, the general convention is for it to be anywhere in the chain and discoverable with errors.Is/errors.As, and I believe the test should follow the convention.
| } | ||
| } | ||
|
|
||
| func TestTableConstants(t *testing.T) { |
There was a problem hiding this comment.
this is a change detector test
## Why? Need database schema and configuration for SQL-based queue implementation to support distributed message processing. ## What? - MySQL schema with 4 tables: messages, offsets, partition leases, and DLQ - Configuration struct with validation for consumer groups, timeouts, and retry policies - Bazel build integration ## Test Plan - Config validation tests pass - Default config values are correct - Invalid configs are rejected appropriately
behinddwalls
force-pushed
the
sql-queue-schema-config
branch
from
7dcf5fb to
fc19779
Compare
sbalabanov
left a comment
sbalabanov
left a comment
There was a problem hiding this comment.
Accepting, but very opinionated that assert.Contains should not be used - can this be removed?
| err := tt.config.Validate() | ||
| if tt.expectError { | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), tt.errorMsg) |
There was a problem hiding this comment.
If error message is not part of the protocol, this makes test fragile to changes for not reason dictated by a change.
If it is part of the protocol, the general convention is for it to be anywhere in the chain and discoverable with errors.Is/errors.As, and I believe the test should follow the convention.
2 participants
Summary
Why?
Need database schema and configuration for SQL-based queue implementation to support distributed message processing.
What?
Test Plan
Issues
Stack