Skip to content

[pulsar-io] Add NSQ Source#8250

Merged
codelipenghui merged 11 commits intoapache:masterfrom
flowchartsman:master
Nov 19, 2020
Merged

[pulsar-io] Add NSQ Source#8250
codelipenghui merged 11 commits intoapache:masterfrom
flowchartsman:master

Conversation

@flowchartsman
Copy link
Contributor

@flowchartsman flowchartsman commented Oct 13, 2020

Motivation

This PR adds a source representing an NSQ topic to be mirrored to a pulsar topic.

Modifications

With the exception of adding it as a module to pulsar-io/pom.xml, there are no modifications to existing code.

Verifying this change

I need some help with this, since it is adapted from a local, standalone repo, and I am having difficulties getting maven to resolve dependencies for other parts of the Pulsar codebase, which prevents me from testing it properly in situ.

This change added tests and can be verified as follows:

This change is largely based off of the Twitter Firehose example, which tests only its config, and I have written similar tests, as well as an extra for the default channel option. Any guidance on further tests is appreciated.

Does this pull request potentially affect one of the following parts:

N/Z

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (following the twitter example, it is documented basically in code)

Note: if I have missed any other links to documentation, please mention them in the comments and I will add them.

Further notes

@flowchartsman
Copy link
Contributor Author

flowchartsman commented Oct 13, 2020

The current issue (aside from a general unfamiliarity with maven and best build practices within the project) is that my IDE (vscode) will not offer the automatic actions on testing that it did in my individual checkout. The only solution I've thought to try so far is to issue a mvn depencency:resolve which fails on project pulsar-kafka-compat-client-test with:

[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  12.633 s
[INFO] Finished at: 2020-10-13T17:01:54-04:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal on project pulsar-kafka-compat-client-test: Could not resolve dependencies for project org.apache.pulsar.tests:pulsar-kafka-compat-client-test:jar:2.7.0-SNAPSHOT: Failure to find org.apache.pulsar.tests:integration:jar:tests:2.7.0-SNAPSHOT in https://repo1.maven.org/maven2 was cached in the local repository, resolution will not be reattempted until the update interval of central has elapsed or updates are forced -> [Help 1]

It seems to resolve my own dependencies fine.

@flowchartsman
Copy link
Contributor Author

Turns out I was building against JDK 14. Used Jenv to install JDK 11, correcting a couple of errors and will issue another push.

@flowchartsman
Copy link
Contributor Author

I believe everything should be in order now. I am unsure why these checks are failing, but they don't seem to be related. Awaiting review.

@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

@flowchartsman Thanks for the great contribution, we will review soon.

@sijie
Copy link
Member

sijie commented Oct 21, 2020

@codelipenghui Can you review this?

@flowchartsman
Copy link
Contributor Author

Pending the review, now is probably a good time to ask about when to add any knobs that the user might want. There are certainly a bunch of NSQ configuration options that could be supplied, this is a pretty bare bones transporter. I am happy to add a few now, though some of them (including auth) would rely on secrets, which I might need some help implementing.

@codelipenghui
Copy link
Contributor

@flowchartsman There are some flaky test that already fixed in master branch, could you please sync with the apache master
branch? Since you are pushing the PR with your master branch, I have sync the apache master branch to your master branch.

@flowchartsman
Copy link
Contributor Author

I believe I may have issued a rebase incorrectly, I will attempt to resolve

@flowchartsman
Copy link
Contributor Author

@codelipenghui I see some tests still failing—are we good, or do I need to make some more changes?

@codelipenghui codelipenghui merged commit fe952a4 into apache:master Nov 19, 2020
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.

4 participants

Comments