[config] Add validation for numeric config fields#449
[config] Add validation for numeric config fields#449Prajwal-banakar wants to merge 2 commits intoapache:mainfrom
Conversation
|
Hi @fresh-borzoni PTAL when you get a chance, Thanks! |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@Prajwal-banakar Ty for the PR, left some comments, PTAL
crates/fluss/src/config.rs
Outdated
| if self.scanner_remote_log_prefetch_num == 0 { | ||
| return Err("scanner_remote_log_prefetch_num must be > 0".to_string()); | ||
| } | ||
| if self.scanner_remote_log_read_concurrency == 0 { |
There was a problem hiding this comment.
We have .max(1) in remote_log.rs, let's remove it from there
There was a problem hiding this comment.
Removed the .max(1) fallback from remote_log.rs
| if self.connect_timeout_ms == 0 { | ||
| return Err("connect_timeout_ms must be > 0".to_string()); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
we need to have these checks as well:
- writer_batch_size <= writer_request_max_size - otherwise batches never drain as they exceed max size defined for request
- writer_batch_size <= writer_buffer_memory_size - or If a single full batch exceeds max_size, it doesn't fit buffer and just keeps piling, we have runtime check for it, but better to validate at config level
There was a problem hiding this comment.
Added the requested checks in config validation
| .map_err(|msg| Error::IllegalArgument { message: msg })?; | ||
| arg.validate_scanner_fetch() | ||
| .map_err(|msg| Error::IllegalArgument { message: msg })?; | ||
| arg.validate_numeric_fields() |
There was a problem hiding this comment.
let's structure this better:
- validate_scanner - to include all scanner related configs
- validate_writer - to inclide writer configs and idempotence checks
- validate_security - sasl checks
so that it's more scoped and it's harder to miss smth
There was a problem hiding this comment.
Refactored validation into scoped methods
crates/fluss/src/config.rs
Outdated
| if self.scanner_remote_log_read_concurrency == 0 { | ||
| return Err("scanner_remote_log_read_concurrency must be > 0".to_string()); | ||
| } | ||
| if self.scanner_log_max_poll_records == 0 { |
There was a problem hiding this comment.
in Java it's allowed, though arguably makes no sense - we need to match, let's drop this for now and file an issue in main repo.
The same for connect_timeout_ms
There was a problem hiding this comment.
I can open a follow-up issue in the main Fluss repo
There was a problem hiding this comment.
I would prefer to get it merged in the main repo first, and then we can add additional stuff here, since we plan to ship more features.
Just remove it and leave some comment for now
Prajwal-banakar
force-pushed
the
Config-validation
branch
from
c9c5948 to
c8d269e
Compare
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@Prajwal-banakar Ty for the update, 2 small comments
- Java parity
- remove validate_idempotency call from writer_client and just leave validate_writer during connection initialization
Mb later we wish to have validation during config creation, but this is out of scope here
crates/fluss/src/config.rs
Outdated
There was a problem hiding this comment.
shall we remove this method then?
There was a problem hiding this comment.
yes, removed
|
Hi @fresh-borzoni thank you for the review, i'll open a issue in main repo, Thanks! |
2 participants
Purpose
Linked issue: close #385
Add a
validate_numeric_fields()method toConfigthat validates all numeric fields have sensible values (e.g. sizes > 0, timeouts >= 0). Validation is called duringFlussConnection::new()alongside the existingvalidate_security()andvalidate_scanner_fetch()checks.Brief change log
validate_numeric_fields()inconfig.rscovering 10 numeric fieldsvalidate_numeric_fields()inFlussConnection::new()inconnection.rsTests
cargo fmt --all
cargo clippy -p fluss-rs --all-features
cargo test -p fluss-rs
API and Format
API: No breaking changes
Storage format: No
Documentation
No doc changes needed.