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

move root chains into sources #1342

Merged
merged 3 commits into from
Sep 21, 2023
Merged

Conversation

rukai
Copy link
Member

@rukai rukai commented Sep 18, 2023

closes #1168
supercedes #1339

After discussion in the canberra office we decided this was the direction we want to take the shotover topology configuration.

The change

Configs like this:

---
sources:
  redis_prod:
    Redis:
      listen_addr: "127.0.0.1:6379"
chain_config:
  redis_chain:
    - RedisSinkSingle:
        remote_address: "127.0.0.1:1111"
        connect_timeout_ms: 3000
source_to_chain_mapping:
  redis_prod: redis_chain

becomes:

---
sources:
  - RedisSource:
      name: "redis_prod"
      listen_addr: "127.0.0.1:6379"
      chain:
        - RedisSinkSingle:
            remote_address: "127.0.0.1:1111"
            connect_timeout_ms: 3000

The root level chains are now part of the source configuration.

alternative approach

In the draft PR #1339 I proposed this:

---
chain_config:
  redis_chain:
    - RedisSource:
        listen_addr: "127.0.0.1:6379"
    - RedisSinkSingle:
        remote_address: "127.0.0.1:1111"
        connect_timeout_ms: 3000

This is actually a lot clearer to read for simple cases.
Its very clear that RedisSource flows into RedisSinkSingle and we have reduced the rightwards drift.
In order to allow this sources are now a subtype of transform called source transforms that must come at the start of the chain.
This is similar to how transforms can be terminating or non-terminating and a transform chain must end in a terminating transform.
So it feels worth the complexity of intermingling transforms/sources to get this clean configuration.

However as soon as we consider subchains the semantics become strange.

---
chain_config:
  redis_chain:
    - RedisSource:
        listen_addr: "127.0.0.1:6379"
        connection_limit: 3000000
    - Tee:
        behavior: FailOnMismatch
        buffer_size: 10000
        chain:
          - QueryTypeFilter:
              filter: Read
          - DebugReturner:
              Redis: "42"
    - DebugReturner:
        Redis: "42"

In root level chains we must start with a source while in a subchain we must never start with a source.
I considered this complexity to push past the point of being worth it and so I abandoned that approach in favor of this PR.

Naming

Previously the source and root level chain were each given their own name.
Now the chain name is derived from the source name since its 1-1.
This does leave the naming a little wonky but will be refined in #337 and #1324

Other Notes

I've left a TODO in the TLS error handling to keep scope of this PR down, pulling on that string will yield many other issues.

@rukai rukai force-pushed the move_root_chains_into_sources branch 4 times, most recently from dd6162a to be7b6fc Compare September 18, 2023 06:12
@rukai rukai marked this pull request as ready for review September 18, 2023 08:24
@rukai rukai requested a review from conorbros September 18, 2023 08:24
@rukai rukai force-pushed the move_root_chains_into_sources branch from be7b6fc to 986b130 Compare September 18, 2023 22:15
@conorbros
Copy link
Member

Should we add a version field to topology.yaml to have a safeguard against using incompatible versions of Shotover with topology file formats? We don't need this at the moment but I'd imagine it will save us from some headaches in the future.

@rukai
Copy link
Member Author

rukai commented Sep 19, 2023

Usually a version field means being backwards compatible across previous versions which would add extra maintenance burden.
But it could be worth it.
I agree, something to consider later on.

@rukai rukai merged commit f9f8d1b into shotover:main Sep 21, 2023
37 checks passed
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.

remove source_to_chain_mapping in topology.yaml format
3 participants