Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New config types in restate-types #1350

Merged
merged 1 commit into from
Apr 4, 2024
Merged

New config types in restate-types #1350

merged 1 commit into from
Apr 4, 2024

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Apr 3, 2024

New config types in restate-types

This introduces a revised configuration structure with the following attributes:

  • Revised names
  • kebab-case serialization
  • Toml dump()
  • new base_dir option (override-able via cli) to set the root for data directories
  • individual data directories are not configurable.
  • More liberal defaults (in some cases)
  • Unifying rocksdb options with ability to override

Default config dump example:

roles = [
    "worker",
    "admin",
    "metadata-store",
]
node-name = "localhost"
cluster-name = "localcluster"
allow-bootstrap = true
base-dir = "/Users/asoli/workspace/restatedev/restate/restate-data"
metadata-store-address = "http://127.0.0.1:5123/"
bind-address = "0.0.0.0:5122"
advertise-address = "http://127.0.0.1:5122/"
shutdown-timeout = "1m"
tracing-filter = "info"
log-filter = "warn,restate=info"
log-format = "pretty"
log-disable-ansi-codes = false
disable-prometheus = false

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

[worker]
internal-queue-length = 64
rocksdb-disable-wal = true
bootstrap-num-partitions = 64

[worker.invoker]
inactivity-timeout = "1m"
abort-timeout = "1m"
message-size-warning = 10485760
tmp-dir = "/tmp/invoker-01HTHYN4JEVVSZDR13SZ1RWWK6"
disable-eager-state = false

[worker.invoker.retry-policy]
type = "exponential"
initial_interval = "50ms"
factor = 2.0
max_attempts = 9223372036854775807
max_interval = "10s"

[admin]
bind-address = "0.0.0.0:9070"
concurrent-api-requests-limit = 9223372036854775807

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

[ingress]
bind-address = "0.0.0.0:8080"
concurrent-api-requests-limit = 9223372036854775807

[ingress.kafka]

[bifrost]
default-provider = "local"

[bifrost.local]
rocksdb-disable-wal = false
writer-commit-batch-size-threshold = 200
writer-commit-time-interval = "13ms"
writer-queue-len = 200
flush-wal-on-commit = true

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

Stack created with Sapling. Best reviewed with ReviewStack.

Copy link

github-actions bot commented Apr 3, 2024

Test Results

 92 files  ± 0   92 suites  ±0   10m 33s ⏱️ -4s
 71 tests  -  8   65 ✅  - 12  2 💤 ±0  4 ❌ +4 
195 runs   - 10  183 ✅  - 16  6 💤 ±0  6 ❌ +6 

For more details on these failures, see this check.

Results for commit ab86f50. ± Comparison against base commit ae19c06.

This pull request removes 12 and adds 4 tests. Note that renamed tests count towards both.
dev.restate.e2e.JavaKafkaIngressTest ‑ handleEventInCounterService(URL, int, IngressClient)
dev.restate.e2e.JavaKafkaIngressTest ‑ handleEventInEventHandler(URL, int, IngressClient)
dev.restate.e2e.JavaNonDeterminismTest ‑ backgroundInvokeWithDifferentTargets
dev.restate.e2e.JavaNonDeterminismTest ‑ callDifferentMethod
dev.restate.e2e.JavaNonDeterminismTest ‑ leftSleepRightCall
dev.restate.e2e.JavaNonDeterminismTest ‑ setDifferentKey
dev.restate.e2e.NodeKafkaIngressTest ‑ handleEventInCounterService(URL, int, IngressClient)
dev.restate.e2e.NodeKafkaIngressTest ‑ handleEventInEventHandler(URL, int, IngressClient)
dev.restate.e2e.NodeNonDeterminismTest ‑ backgroundInvokeWithDifferentTargets
dev.restate.e2e.NodeNonDeterminismTest ‑ callDifferentMethod
…
dev.restate.e2e.JavaKafkaIngressTest ‑ initializationError
dev.restate.e2e.JavaNonDeterminismTest ‑ initializationError
dev.restate.e2e.NodeKafkaIngressTest ‑ initializationError
dev.restate.e2e.NodeNonDeterminismTest ‑ initializationError

♻️ This comment has been updated with latest results.

pub histogram_inactivity_timeout: Option<humantime::Duration>,

#[serde(flatten)]
pub service_client: ServiceClientOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome! i wonder if we need to flatten here though vs having a field name like client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pro flattening (3 levels is already a stretch), but we can easily change this before release if we feel that client makes ergonomics better. The change won't impact the code structure which is good.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok im relaxed !

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks for creating the new config types @AhmedSoliman. They look good to me :-) +1 for merging.

crates/types/src/config/common.rs Outdated Show resolved Hide resolved
This introduces a revised configuration structure with the following attributes:
- Revised names
- kebab-case serialization
- Toml dump()
- new base_dir option (override-able via cli) to set the root for data directories
- individual data directories are not configurable.
- More liberal defaults (in some cases)
- Unifying rocksdb options with ability to override


Default config dump example:
```toml
roles = [
    "worker",
    "admin",
    "metadata-store",
]
node-name = "localhost"
cluster-name = "localcluster"
allow-bootstrap = true
base-dir = "/Users/asoli/workspace/restatedev/restate/restate-data"
metadata-store-address = "http://127.0.0.1:5123/"
bind-address = "0.0.0.0:5122"
advertise-address = "http://127.0.0.1:5122/"
shutdown-timeout = "1m"
tracing-filter = "info"
log-filter = "warn,restate=info"
log-format = "pretty"
log-disable-ansi-codes = false
disable-prometheus = false

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

[worker]
internal-queue-length = 64
rocksdb-disable-wal = true
bootstrap-num-partitions = 64

[worker.invoker]
inactivity-timeout = "1m"
abort-timeout = "1m"
message-size-warning = 10485760
tmp-dir = "/tmp/invoker-01HTHYN4JEVVSZDR13SZ1RWWK6"
disable-eager-state = false

[worker.invoker.retry-policy]
type = "exponential"
initial_interval = "50ms"
factor = 2.0
max_attempts = 9223372036854775807
max_interval = "10s"

[admin]
bind-address = "0.0.0.0:9070"
concurrent-api-requests-limit = 9223372036854775807

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

[ingress]
bind-address = "0.0.0.0:8080"
concurrent-api-requests-limit = 9223372036854775807

[ingress.kafka]

[bifrost]
default-provider = "local"

[bifrost.local]
rocksdb-disable-wal = false
writer-commit-batch-size-threshold = 200
writer-commit-time-interval = "13ms"
writer-queue-len = 200
flush-wal-on-commit = true

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

```
Copy link
Contributor

@jackkleeman jackkleeman left a comment

Choose a reason for hiding this comment

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

have mainly reviewed the service client aspects but seems huge improvement for me overall. thanks!

@AhmedSoliman AhmedSoliman merged commit 4c5c347 into main Apr 4, 2024
8 of 11 checks passed
@AhmedSoliman AhmedSoliman deleted the pr1350 branch April 4, 2024 08:53
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