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

Validate topology.yaml #263

Merged
merged 23 commits into from Oct 26, 2021
Merged

Validate topology.yaml #263

merged 23 commits into from Oct 26, 2021

Conversation

conorbros
Copy link
Member

@conorbros conorbros commented Oct 12, 2021

  • every transform chain ends in a terminating transform
  • a terminating transform is not followed by a non-terminating transform
    transforms are compatible with each other e.g. RedisTimestampTagger -> CassandraDestination will make a follow up PR for this as this one is large enough

@conorbros conorbros changed the title [WIP] Validate topology.yaml Validate topology.yaml Oct 14, 2021
@conorbros conorbros requested a review from rukai October 14, 2021 06:44
@conorbros
Copy link
Member Author

Example output is below:

Topology errors:
redis_chain:
  Terminating transform "Null" is not last in chain
  Terminating transform "Null" is not last in chain
  Non-terminating transform "Printer" is last in chain

other_chain:
    chain_3:
        sub_chain_2:
          Terminating transform "Null" is not last in chain
    chain_1:
      Terminating transform "Null" is not last in chain
      Non-terminating transform "Printer" is last in chain
    chain_2:
      Terminating transform "Null" is not last in chain


thread 'Shotover-Proxy-Thread' panicked at 'Topology is not valid', shotover-proxy/src/config/topology.rs:138:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Added to the Transform trait:

  • is_terminating: default implementation is set to false because it's far more common. Terminating transforms override this.
  • validate: default implementation checks whether the transform's position in the chain is valid using the is_terminating method. Transforms that require more logic (such as running their subchain's validation) override this.

Added to TransformChain:

  • validate which will call each Transform's validate method.

Errors are passed back using a Vec<String>, Runner will check to see if this is not empty, dumps to stderr and panics.

Copy link
Member

@rukai rukai left a comment

Choose a reason for hiding this comment

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

Heres some feedback, but I think its best to keep focusing on #275 before we come back to this.

shotover-proxy/src/transforms/chain.rs Outdated Show resolved Hide resolved
shotover-proxy/src/config/topology.rs Outdated Show resolved Hide resolved
shotover-proxy/src/config/topology.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

1 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1352605458

shotover-proxy/src/config/topology.rs Outdated Show resolved Hide resolved
shotover-proxy/src/config/topology.rs Outdated Show resolved Hide resolved
shotover-proxy/src/transforms/chain.rs Outdated Show resolved Hide resolved
shotover-proxy/src/transforms/chain.rs Show resolved Hide resolved
shotover-proxy/src/transforms/chain.rs Outdated Show resolved Hide resolved
shotover-proxy/src/transforms/mod.rs Outdated Show resolved Hide resolved
shotover-proxy/src/transforms/mod.rs Show resolved Hide resolved
@rukai rukai mentioned this pull request Oct 19, 2021
@github-actions
Copy link

1 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1357988606

@github-actions
Copy link

2 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1362058174

@github-actions
Copy link

2 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1362662111

@github-actions
Copy link

1 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1366164776

@github-actions
Copy link

1 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1366591294

@github-actions
Copy link

1 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1366611104

@github-actions
Copy link

3 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1370307857

Copy link
Member

@rukai rukai left a comment

Choose a reason for hiding this comment

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

So currently we have 3 layers of tests:

  • highest level integration test - does shotover shutdown when validation fails.
  • mid level test - testing that we get specific nicely formatted errors in response to validation failures.
  • lowest level unit tests - test specific transform/chain validation implementation to ensure we get full coverage of its validation implementation.

Im happy for all of these layers to exist.
However, duplicate tests add a lot of maintenance burden so its good to avoid when we can.
I'm noticing that the lowest level unit tests do a lot of testing of other transforms/chain validation logic, such tests should be moved into the unittests for the other transform/chain or moved into a mid level test.

Would like to hear your thoughts before you go rewriting lots of stuff.
Maybe you had your own reasoning for how you wrote them?

shotover-proxy/src/config/topology.rs Outdated Show resolved Hide resolved
shotover-proxy/src/config/topology.rs Outdated Show resolved Hide resolved
shotover-proxy/src/config/topology.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

1 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1378879865

@github-actions
Copy link

4 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1379219498

@github-actions
Copy link

1 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1379259994

@conorbros conorbros mentioned this pull request Oct 25, 2021
@github-actions
Copy link

4 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1379406911

shotover-proxy/src/config/topology.rs Outdated Show resolved Hide resolved
shotover-proxy/src/config/topology.rs Show resolved Hide resolved
shotover-proxy/src/transforms/mod.rs Outdated Show resolved Hide resolved
shotover-proxy/src/transforms/mod.rs Outdated Show resolved Hide resolved
shotover-proxy/src/transforms/parallel_map.rs Outdated Show resolved Hide resolved
shotover-proxy/src/transforms/redis/cache.rs Outdated Show resolved Hide resolved
shotover-proxy/src/config/topology.rs Show resolved Hide resolved
@conorbros
Copy link
Member Author

Probably not, I'd imagine some of them are quite out of date. I can make another PR for updating the examples and testing any that are not already run as part of tests.

@benbromhead
Copy link
Member

sounds good

@rukai
Copy link
Member

rukai commented Oct 26, 2021

good catch with the rebase :)

@github-actions
Copy link

1 benchmarks reported regressed performance. Please check the benchmark workflow logs for details: https://github.com/shotover/shotover-proxy/actions/runs/1383728363

@conorbros conorbros merged commit 2d72436 into shotover:main Oct 26, 2021
@conorbros conorbros deleted the validate-topology branch October 27, 2021 00:41
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.

None yet

3 participants