Skip to content

feat: change primitives - #181

Merged
abergasov merged 2 commits into
mainfrom
_change_types
Aug 13, 2026
Merged

feat: change primitives#181
abergasov merged 2 commits into
mainfrom
_change_types

Conversation

@abergasov

@abergasov abergasov commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features

    • Configuration values now support unsigned size settings and higher-precision decimal values.
    • Added support for consistently hashing unsigned integers and 64-bit decimal values.
  • Bug Fixes

    • Updated remote configuration hashing to accurately reflect the revised setting types.
    • Improved consistency between default and dynamically applied configuration values.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes the primitive types of several sharding/message config fields in the shared optimum-common library: RandomMessageSize and ShardFactor move from int64 to uint32, and PublisherShardMultiplier and ForwardShardThreshold move from float32 to float64. These changes are applied consistently across OptimumConfig and DynamicConfig, and two new hash helpers (WriteFloat64, WriteUint32) are added so HashRemoteConfig can hash the new types. The config loader already supports uint*/float64 kinds, so defaults/env/flag parsing continues to work.

Changes:

  • Switch four coding/message config fields to uint32/float64 in both OptimumConfig and DynamicConfig.
  • Add WriteUint32 and WriteFloat64 hash helpers and use them in HashRemoteConfig.
  • Update tests (type assertions in ToMap, and regenerated SHA-256 hash expectations).

Note: because these are exported fields of a shared "common" package, the type change is a breaking API change for any downstream consumer that references them as int64/float32; ensure dependents are updated in lockstep.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/hash/hashutil.go Adds WriteFloat64 and WriteUint32 helpers (missing doc comments).
pkg/entities/opt_p2p.go Changes four OptimumConfig field types to uint32/float64.
pkg/entities/opt_p2p_test.go Updates type assertions to match new field types.
pkg/entities/dynamic_config.go Changes matching DynamicConfig field types and updates HashRemoteConfig to use new helpers.
pkg/entities/dynamic_config_test.go Updates ToMap type assertions and regenerated hash expectations.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkg/hash/hashutil.go
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 918ba278-1563-4d3b-83b0-5614e506e3e1

📥 Commits

Reviewing files that changed from the base of the PR and between 94356b7 and ec391dc.

📒 Files selected for processing (1)
  • pkg/hash/hashutil.go

📝 Walkthrough

Walkthrough

The change updates four configuration fields from signed integers and single-precision floats to uint32 and float64. Remote configuration hashing now uses matching little-endian encodings. Hash utilities provide WriteFloat64 and WriteUint32. Configuration tests update numeric expectations and expected hashes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Mergeability Score: ⚪ Minimal · up to ec391

The change is limited to primitive-related updates and no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: hpsing

🚥 Pre-merge checks | ✅ 6 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title uses a valid type but omits the required domain/pkg scope and uses a vague non-imperative summary. Rename it to feat(<domain/pkg>): without trailing punctuation, such as feat(entities): use precise config numeric types.
Scope Discipline ⚠️ Warning The PR diff against origin/main includes pkg/syncx/broadcaster.go and its tests, adding BroadcastTry and buffered listeners from separate #179; these are unrelated to changing configuration primiti... Remove the unrelated pkg/syncx changes from this PR, or provide an explicit scope explanation and move them to a separate PR.
Behavior Safety ⚠️ Warning BroadcastTry reads activeListeners before locking, while registration and unregistration write it under b.mu; concurrent listener changes introduce a data race. Move the drops map allocation after acquiring b.mu, or derive its capacity from len(b.messages) while the lock is held.
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Over-Engineering ✅ Passed The diff introduces no cache or gratuitous helper layer; WriteUint32/WriteFloat64 mirror existing hash writers, and tests cover observable map and hash outputs rather than internals.
Security ✅ Passed The PR changes configuration types and deterministic hash serialization only; review found no injection, credential, secret-logging, or weak-crypto behavior introduced.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch _change_types

Comment @coderabbitai help to get the list of available commands.

@abergasov
abergasov enabled auto-merge (squash) August 13, 2026 14:50
@abergasov
abergasov merged commit 96e3a0e into main Aug 13, 2026
7 checks passed
@abergasov
abergasov deleted the _change_types branch August 13, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants