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

Add initial nats source connector #2

Merged
merged 8 commits into from
Feb 9, 2024

Conversation

pinkforest
Copy link
Contributor

@pinkforest pinkforest commented Dec 11, 2023

Needs to be added:

  • bats - need to wire the CLI into CI
  • more documentation

Can be added later:

Copy link

@digikata digikata left a comment

Choose a reason for hiding this comment

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

Looks simple and clean in the code, some minor loose ends in the CI, and we (infinyon) would need to hook up a cloud publish workflow if we want to this in the hub.

async fn start(config: NatsConfig, producer: TopicProducer) -> Result<()> {
debug!(?config);
let source = NatsSource::new(&config)?;
let mut stream = source.connect(None).await?;

Choose a reason for hiding this comment

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

Depending on the long run behavior of Nats/fluvio, it may be interesting to have a larger loop around the connect .. next to implement a longer term re-connect strategy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we just crashed the connector and there wasn't really good feedback loop to cluster to handle failing connectors.. it would be good to have some kind of strategy e.g. also observability of these failures

Copy link

@galibey galibey left a comment

Choose a reason for hiding this comment

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

Overall it looks good, just need to remove a couple of unused pieces.

src/source.rs Show resolved Hide resolved
src/source.rs Outdated Show resolved Hide resolved
src/source.rs Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
cloud_e2e_test:
bats ./tests/cloud-consumes-data-from-nats.bats

test_fluvio_install:
Copy link

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in every connector like that - https://github.com/infinyon/http-source-connector/blob/main/Makefile#L19

Should this be removed ?

Copy link

Choose a reason for hiding this comment

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

Ideally, yes, because it makes no sense to keep it here.

I will remove it from http-source when I have a chance.

src/source.rs Show resolved Hide resolved
Copy link

@galibey galibey left a comment

Choose a reason for hiding this comment

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

LGTM

@digikata digikata merged commit b385c89 into fluvio-connectors:main Feb 9, 2024
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