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

[ISSUE #4869] Add Webhook support for HTTP Source Connector #4913

Merged
merged 14 commits into from
Jun 27, 2024

Conversation

cnzakii
Copy link
Contributor

@cnzakii cnzakii commented May 26, 2024

Fixes #4869

Motivation

[Feature] Add Webhook support for HTTP Source Connector #4869

Modifications

  1. Support Github webhook
  2. Support Common webhook
  3. Some other optimizations

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@cnzakii
Copy link
Contributor Author

cnzakii commented May 26, 2024

@Pil0tXia PTAL

Copy link
Member

@Pil0tXia Pil0tXia left a comment

Choose a reason for hiding this comment

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

I haven't finished reading it yet, I need to travel tomorrow and the next day. I will continue to review when I have time.

@cnzakii
Copy link
Contributor Author

cnzakii commented May 28, 2024

I haven't finished reading it yet, I need to travel tomorrow and the next day. I will continue to review when I have time.

No problem, open-source work is not the main focus for most people, but don't forget to continue the code review.🤪

@Pil0tXia Pil0tXia added the waiting for contributor PR is awaiting the contributor's response to the review for further evaluation. label Jun 12, 2024
Copy link
Member

@Pil0tXia Pil0tXia left a comment

Choose a reason for hiding this comment

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

Could you please let me know if the logical order of the information has not been submitted yet?

BTW, please resolve conflicts. ☺️

@cnzakii
Copy link
Contributor Author

cnzakii commented Jun 26, 2024

BTW, please resolve conflicts.

OK

Could you please let me know if the logical order of the information has not been submitted yet?

First, for the GitHub protocol, I have imposed a restriction where the source can only receive either forms or JSON, but not both simultaneously. This ensures that all data is either parsed (form) or not parsed (JSON). Consequently, this should ensure that data undergoes the same processing logic, thereby maintaining the order.

For the thread-safe queue, I have currently encapsulated CircularFifoQueue and made it thread-safe using the synchronized keyword. The reason for not using some convenient utility classes is that the fetchRange method is unique to the current queue, and using utility classes cannot guarantee the thread safety of this method.

# Conflicts:
#	eventmesh-connectors/eventmesh-connector-http/src/main/java/org/apache/eventmesh/connector/http/source/connector/HttpSourceConnector.java
@Pil0tXia Pil0tXia added ready for review PR is waiting for reviewer's approval or opinion (used as a strong reminder) and removed waiting for contributor PR is awaiting the contributor's response to the review for further evaluation. labels Jun 26, 2024
Copy link
Member

@Pil0tXia Pil0tXia left a comment

Choose a reason for hiding this comment

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

Good job, LGTM.

The assignee of #4825 has already started working at a company and doesn't have time to develop this issue. I can assign this issue to you instead. Are you interested?

@cnzakii
Copy link
Contributor Author

cnzakii commented Jun 26, 2024

The assignee of #4825 has already started working at a company and doesn't have time to develop this issue. I can assign this issue to you instead. Are you interested?

Although I would very much like to claim and complete this issue, I currently have other work to finish. You can try assigning it to other contributors first. If I have free time and the issue hasn't been resolved, I will claim and solve it.

Copy link
Contributor

@xwm1992 xwm1992 left a comment

Choose a reason for hiding this comment

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

LGTM

@xwm1992 xwm1992 merged commit 996c439 into apache:master Jun 27, 2024
10 checks passed
@cnzakii cnzakii deleted the feat/4869 branch June 27, 2024 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PR is waiting for reviewer's approval or opinion (used as a strong reminder)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add Webhook support for HTTP Source Connector
3 participants