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

refactor: depend on numaflow to use native nats configuration struct #3

Closed
wants to merge 2 commits into from

Conversation

KeranYang
Copy link
Contributor

@KeranYang KeranYang commented Sep 13, 2023

This change brings the following benefits:

  1. Single source of truth on the NATS configuration struct. In the case of adding new features to the NATS source, to ensure NATS UDSource is in sync with the Numaflow platform NATS source, we now just need to update the dependency version of Numaflow instead of manually copying new attributes.
  2. Auto-generated guide on each of the NATS configurations. We now use the native Numaflow guide API reference to show users how to specify a NATS UDSource configuration.
  3. Ease to integrate with the current user-friendly numaflow NATS spec. On the Numaflow platform side, the controller can just parse the native NatsSource obj to JSON format and mount it to volume. NATS UDSource has no problem interpreting the JSON, saving one type translation.

Not so good:

One can argue that this brings in a circular dependency where NATS UDSource depends on Numaflow platform code while Numaflow depends on the NATS UDSource to run its built-in NATS source. However, the second dependency is simply a dependency on the image instead of a code dependency so it should be fine.

Signed-off-by: Keran Yang <yangkr920208@gmail.com>
.
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
@KeranYang KeranYang marked this pull request as ready for review September 13, 2023 21:05
@KeranYang KeranYang requested review from juliev0, whynowy and a team September 13, 2023 21:06
@@ -6,21 +6,31 @@ require (
github.com/google/uuid v1.3.0
github.com/nats-io/nats-server/v2 v2.9.22
github.com/nats-io/nats.go v1.28.0
github.com/numaproj/numaflow v0.10.0
Copy link

Choose a reason for hiding this comment

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

this doesn't seem right. what if you were writing the source in python? 😄

Copy link
Contributor Author

@KeranYang KeranYang Sep 14, 2023

Choose a reason for hiding this comment

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

+1. Do you think if we move &v1alpha1.NatsSource from numaflow to this repo and let numaflow depends on this repo, it will be a better approach? As UDSource OWNS the definition of configurations and platform has minimal knowledge of it.

@KeranYang KeranYang marked this pull request as draft September 14, 2023 17:05
@KeranYang KeranYang closed this Sep 25, 2023
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

2 participants