Configuration support for human-readable bytes #1455

AhmedSoliman · 2024-04-23T15:59:51Z

Configuration support for human-readable bytes

Add human-readable bytes support to the configuration
Full review of naming
Remove values the default to MAX and use Option instead
Various fixes for better default values and hiding some internal options

Default config:

roles = [
    "worker",
    "admin",
    "metadata-store",
]
cluster-name = "localcluster"
allow-bootstrap = true
metadata-store-address = "http://127.0.0.1:5123/"
bind-address = "0.0.0.0:5122"
advertised-address = "http://127.0.0.1:5122/"
shutdown-timeout = "1m"
tracing-filter = "info"
log-filter = "info"
log-format = "pretty"
log-disable-ansi-codes = false
disable-prometheus = false
rocksdb-total-memory-size = "4.0 GB"
rocksdb-total-memtables-size = "0 B"
rocksdb-high-priority-bg-threads = 2

[http-keep-alive-options]
interval = "40s"
timeout = "20s"

[worker]
internal-queue-length = 64
rocksdb-write-buffer-size = "256.0 MB"
rocksdb-max-total-wal-size = "2.0 GB"
rocksdb-disable-wal = true
rocksdb-disable-statistics = false
rocksdb-max-background-jobs = 12
rocksdb-compaction-readahead-size = "2.0 MB"
rocksdb-statistics-level = "except-detailed-timers"
bootstrap-num-partitions = 64

[worker.invoker]
inactivity-timeout = "1m"
abort-timeout = "1m"
message-size-warning = "10.0 MB"

[worker.invoker.retry-policy]
type = "exponential"
initial-interval = "50ms"
factor = 2.0
max-interval = "10s"

[admin]
bind-address = "0.0.0.0:9070"

[admin.query-engine]
pgsql-bind-address = "0.0.0.0:9071"

[ingress]
bind-address = "0.0.0.0:8080"
kafka-clusters = []

[bifrost]
default-provider = "local"

[bifrost.local]
rocksdb-write-buffer-size = "256.0 MB"
rocksdb-max-total-wal-size = "2.0 GB"
rocksdb-disable-wal = false
rocksdb-disable-statistics = false
rocksdb-max-background-jobs = 12
rocksdb-compaction-readahead-size = "2.0 MB"
rocksdb-statistics-level = "except-detailed-timers"
writer-batch-commit-count = 500
writer-batch-commit-duration = "5ns"
sync-wal-before-ack = true

[metadata-store]
bind-address = "0.0.0.0:5123"
request-queue-length = 32

[metadata-store.rocksdb]
rocksdb-write-buffer-size = "256.0 MB"
rocksdb-max-total-wal-size = "2.0 GB"
rocksdb-disable-wal = true
rocksdb-disable-statistics = false
rocksdb-max-background-jobs = 12
rocksdb-compaction-readahead-size = "2.0 MB"
rocksdb-statistics-level = "except-detailed-timers"

github-actions · 2024-04-23T16:54:29Z

Test Results

95 files ±0 95 suites ±0 7m 27s ⏱️ -38s
82 tests ±0 80 ✅ ±0 2 💤 ±0 0 ❌ ±0
212 runs ±0 206 ✅ ±0 6 💤 ±0 0 ❌ ±0

Results for commit e330750. ± Comparison against base commit 8139946.

♻️ This comment has been updated with latest results.

jackkleeman

Thanks for the heads up! I don't think we are setting any of these in cloud yet but let me know if we should be

tillrohrmann

Thanks for serializing our memory sizes in human readable form. This is a really cool improvement! I had a few questions concerning the Serialize/DeserializeAs implementations and whether to enforce non-zero constraints directly on the fields of an option struct.

tillrohrmann · 2024-04-24T07:32:30Z

crates/serde-util/src/byte_count.rs

+
+struct ByteCountVisitor<const CAN_BE_ZERO: bool>;
+
+// because we do what appears to be runtime-check fo the const CAN_BE_ZERO but


Suggested change

// because we do what appears to be runtime-check fo the const CAN_BE_ZERO but

// because we do what appears to be runtime-check of the const CAN_BE_ZERO but

tillrohrmann · 2024-04-24T07:42:12Z

crates/serde-util/src/byte_count.rs

+impl<const CAN_BE_ZERO: bool> From<u64> for ByteCount<CAN_BE_ZERO> {
+    fn from(value: u64) -> Self {
+        ByteCount(value)
+    }
+}


Shouldn't this only be implemented for ByteCount<true>?

tillrohrmann · 2024-04-24T07:46:36Z

crates/serde-util/src/byte_count.rs

+        D: Deserializer<'de>,
+    {
+        if deserializer.is_human_readable() {
+            deserializer.deserialize_any::<ByteCountVisitor<CAN_BE_ZERO>>(ByteCountVisitor)


Why don't we hint that the expected type is a str? Is it to support that users can specify it as a pure number rocksdb-total-memory-size = 0?

Yes, both are supported.

tillrohrmann · 2024-04-24T07:54:54Z

crates/serde-util/src/byte_count.rs

+        #[track_caller]
+        fn check(s: &str) {
+            assert_eq!(
+                serde_json::from_str::<Config>(s).unwrap().0,
+                s.parse().unwrap()
+            );
+        }


Is this method different from check_str? Looks like I can replace check with check_str in the test below.

check_str adds surrounds the value with \" so it becomes json string.

Cool, I didn't know that debug output of str includes the "". Thanks for the clarification!

tillrohrmann · 2024-04-24T07:55:34Z

crates/serde-util/src/byte_count.rs

+        // can deserialize directly from numbers
+        assert_eq!(serde_json::from_str::<Config>("5").unwrap().0, ByteCount(5));


Isn't this part covered by check("123")?

tillrohrmann · 2024-04-24T08:07:06Z

crates/serde-util/src/byte_count.rs

+impl<const CAN_BE_ZERO: bool> SerializeAs<u64> for ByteCount<CAN_BE_ZERO> {
+    fn serialize_as<S>(source: &u64, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        Self(*source).serialize(serializer)
+    }
+}


Shouldn't all SerializeAs/DeserializeAs implementations for zero-able types be only implemented for ByteCount<true>? Otherwise, when deserializing a 0 value using ByteCount<false>, it will probably crash.

Ok, I see how you are using it in the options structs to enforce that a u64 is actually non-zero when deserializing it. I am wondering whether we shouldn't change the type from u64 to NonZeroU64 for those fields instead. The reason is that we can now construct a CommonOptions with 0 valued fields which we can serialize but fail to deserialize because of the non-zero constraint that is only enforced at deserialization time.

tillrohrmann · 2024-04-24T08:21:41Z

crates/types/src/config/common.rs

+    /// The memory size used for rocksdb caches.
+    #[serde_as(as = "ByteCount<false>")]
+    #[cfg_attr(feature = "schemars", schemars(with = "ByteCount<false>"))]
+    pub rocksdb_total_memory_size: u64,


Related to my previous comment. If we want this field to be non zero, should we change it to NonZeroU64?

tillrohrmann · 2024-04-24T08:26:50Z

crates/types/src/retries.rs

@@ -124,25 +125,25 @@ impl Default for RetryPolicy {
 }

 impl RetryPolicy {
-    pub fn fixed_delay(interval: Duration, max_attempts: usize) -> Self {
+    pub fn fixed_delay(interval: Duration, max_attempts: Option<usize>) -> Self {


Should max_attempts have the type Option<NonZeroUsize> to avoid the expect?

All those were left in an attempt to stop the refactoring from cascading down tbh. My goal was to be able to ship this by end of day. I can definitely go over those types and follow the rabbit hole :)

tillrohrmann · 2024-04-24T08:26:57Z

crates/types/src/retries.rs

        }
    }

    pub fn exponential(
        initial_interval: Duration,
        factor: f32,
-        max_attempts: usize,
+        max_attempts: Option<usize>,


Same question here.

Just to avoid refactoring the high number of call sites at this PR.

- Add human-readable bytes support to the configuration - Full review of naming - Remove values the default to MAX and use Option instead - Various fixes for better default values and hiding some internal options Default config: ```toml roles = [ "worker", "admin", "metadata-store", ] cluster-name = "localcluster" allow-bootstrap = true metadata-store-address = "http://127.0.0.1:5123/" bind-address = "0.0.0.0:5122" advertised-address = "http://127.0.0.1:5122/" shutdown-timeout = "1m" tracing-filter = "info" log-filter = "info" log-format = "pretty" log-disable-ansi-codes = false disable-prometheus = false rocksdb-total-memory-size = "4.0 GB" rocksdb-total-memtables-size = "0 B" rocksdb-high-priority-bg-threads = 2 [http-keep-alive-options] interval = "40s" timeout = "20s" [worker] internal-queue-length = 64 rocksdb-write-buffer-size = "256.0 MB" rocksdb-max-total-wal-size = "2.0 GB" rocksdb-disable-wal = true rocksdb-disable-statistics = false rocksdb-max-background-jobs = 12 rocksdb-compaction-readahead-size = "2.0 MB" rocksdb-statistics-level = "except-detailed-timers" bootstrap-num-partitions = 64 [worker.invoker] inactivity-timeout = "1m" abort-timeout = "1m" message-size-warning = "10.0 MB" [worker.invoker.retry-policy] type = "exponential" initial-interval = "50ms" factor = 2.0 max-interval = "10s" [admin] bind-address = "0.0.0.0:9070" [admin.query-engine] pgsql-bind-address = "0.0.0.0:9071" [ingress] bind-address = "0.0.0.0:8080" kafka-clusters = [] [bifrost] default-provider = "local" [bifrost.local] rocksdb-write-buffer-size = "256.0 MB" rocksdb-max-total-wal-size = "2.0 GB" rocksdb-disable-wal = false rocksdb-disable-statistics = false rocksdb-max-background-jobs = 12 rocksdb-compaction-readahead-size = "2.0 MB" rocksdb-statistics-level = "except-detailed-timers" writer-batch-commit-count = 500 writer-batch-commit-duration = "5ns" sync-wal-before-ack = true [metadata-store] bind-address = "0.0.0.0:5123" request-queue-length = 32 [metadata-store.rocksdb] rocksdb-write-buffer-size = "256.0 MB" rocksdb-max-total-wal-size = "2.0 GB" rocksdb-disable-wal = true rocksdb-disable-statistics = false rocksdb-max-background-jobs = 12 rocksdb-compaction-readahead-size = "2.0 MB" rocksdb-statistics-level = "except-detailed-timers" ```

AhmedSoliman · 2024-04-24T10:00:35Z

@tillrohrmann thanks for the great review. I've followed your advice and the I like how the new version provides better guards by the compiler. The annoyance with NonZero* is that there is no compiler-time check on literal assignments, so you'll see a few unwrap()s and I also limited the cascading effect of using those types, but at least, the configuration is fully covered.

tillrohrmann

Great work @AhmedSoliman. It is a really good improvement for our users :-) +1 for merging.

tillrohrmann · 2024-04-24T11:23:18Z

crates/serde-util/src/byte_count.rs

+        D: Deserializer<'de>,
+    {
+        if deserializer.is_human_readable() {
+            deserializer.deserialize_any::<ByteCountVisitor<CAN_BE_ZERO>>(ByteCountVisitor)


tillrohrmann · 2024-04-24T11:27:09Z

crates/serde-util/src/byte_count.rs

+        #[track_caller]
+        fn check(s: &str) {
+            assert_eq!(
+                serde_json::from_str::<Config>(s).unwrap().0,
+                s.parse().unwrap()
+            );
+        }


Cool, I didn't know that debug output of str includes the "". Thanks for the clarification!

AhmedSoliman mentioned this pull request Apr 23, 2024

[RocksDb] Dispatch bifrost writes to bg threads automatically #1438

Merged

AhmedSoliman force-pushed the pr1455 branch 3 times, most recently from 3d48123 to 1f1dedf Compare April 23, 2024 16:23

AhmedSoliman marked this pull request as ready for review April 23, 2024 16:24

AhmedSoliman requested a review from tillrohrmann April 23, 2024 16:24

AhmedSoliman force-pushed the pr1455 branch from 1f1dedf to 60b9bd0 Compare April 23, 2024 16:26

AhmedSoliman mentioned this pull request Apr 23, 2024

Update configuration-related sections restatedev/documentation#359

Merged

AhmedSoliman force-pushed the pr1455 branch from 60b9bd0 to a71ff58 Compare April 23, 2024 17:10

AhmedSoliman requested a review from jackkleeman April 23, 2024 17:11

jackkleeman approved these changes Apr 23, 2024

View reviewed changes

AhmedSoliman force-pushed the pr1455 branch from a71ff58 to afb2d19 Compare April 24, 2024 07:36

tillrohrmann reviewed Apr 24, 2024

View reviewed changes

AhmedSoliman mentioned this pull request Apr 24, 2024

Remove RocksDb writer thread and disable WAL #1456

Merged

AhmedSoliman force-pushed the pr1455 branch from afb2d19 to 1939ddb Compare April 24, 2024 09:50

AhmedSoliman force-pushed the pr1455 branch from 1939ddb to e330750 Compare April 24, 2024 09:56

AhmedSoliman requested a review from tillrohrmann April 24, 2024 10:00

AhmedSoliman mentioned this pull request Apr 24, 2024

config schema sync restatedev/documentation#360

Merged

tillrohrmann approved these changes Apr 24, 2024

View reviewed changes

AhmedSoliman merged commit ba593f8 into main Apr 24, 2024
13 checks passed

AhmedSoliman deleted the pr1455 branch April 24, 2024 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration support for human-readable bytes #1455

Configuration support for human-readable bytes #1455

AhmedSoliman commented Apr 23, 2024 •

edited

Loading

github-actions bot commented Apr 23, 2024 •

edited

Loading

jackkleeman left a comment

tillrohrmann left a comment

tillrohrmann Apr 24, 2024

tillrohrmann Apr 24, 2024

tillrohrmann Apr 24, 2024

AhmedSoliman Apr 24, 2024

tillrohrmann Apr 24, 2024

tillrohrmann Apr 24, 2024

AhmedSoliman Apr 24, 2024

tillrohrmann Apr 24, 2024

tillrohrmann Apr 24, 2024

tillrohrmann Apr 24, 2024

tillrohrmann Apr 24, 2024

tillrohrmann Apr 24, 2024

tillrohrmann Apr 24, 2024

AhmedSoliman Apr 24, 2024

tillrohrmann Apr 24, 2024

AhmedSoliman Apr 24, 2024

AhmedSoliman commented Apr 24, 2024

tillrohrmann left a comment

tillrohrmann Apr 24, 2024

tillrohrmann Apr 24, 2024


		struct ByteCountVisitor<const CAN_BE_ZERO: bool>;

		// because we do what appears to be runtime-check fo the const CAN_BE_ZERO but

		// can deserialize directly from numbers
		assert_eq!(serde_json::from_str::<Config>("5").unwrap().0, ByteCount(5));

Configuration support for human-readable bytes #1455

Configuration support for human-readable bytes #1455

Conversation

AhmedSoliman commented Apr 23, 2024 • edited Loading

github-actions bot commented Apr 23, 2024 • edited Loading

Test Results

jackkleeman left a comment

Choose a reason for hiding this comment

tillrohrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhmedSoliman commented Apr 24, 2024

tillrohrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhmedSoliman commented Apr 23, 2024 •

edited

Loading

github-actions bot commented Apr 23, 2024 •

edited

Loading