Skip to content

feat(pubsub): support kafka#7032

Merged
spacewander merged 20 commits into
apache:masterfrom
bzp2010:feat-pubsub-kafka
May 16, 2022
Merged

feat(pubsub): support kafka#7032
spacewander merged 20 commits into
apache:masterfrom
bzp2010:feat-pubsub-kafka

Conversation

@bzp2010

@bzp2010 bzp2010 commented May 12, 2022

Copy link
Copy Markdown
Contributor

Description

Provide kafka implementation on pubsub framework.

Split from #6995

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@bzp2010
bzp2010 force-pushed the feat-pubsub-kafka branch from a8eca0a to ac25c0b Compare May 12, 2022 20:15
@bzp2010

bzp2010 commented May 12, 2022

Copy link
Copy Markdown
Contributor Author

Update

There are still some test cases that have not been debug passed yet.

@bzp2010 bzp2010 self-assigned this May 13, 2022
@bzp2010
bzp2010 marked this pull request as ready for review May 13, 2022 05:25
@bzp2010 bzp2010 changed the title feat: support kafka pubsub feat(pubsub): support kafka May 13, 2022
membphis
membphis previously approved these changes May 15, 2022

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread t/admin/upstream5.t
# See the License for the specific language governing permissions and
# limitations under the License.
#
use t::APISIX 'no_plan';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

t/admin/upstream5.t we need a meaningful name

@bzp2010 bzp2010 May 15, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@membphis This is just because the previous upstreamX test contains too many lines, and it does not point to the upstream scheme test of "Kafka", future upstream-related tests can also be added to this file (this test was moved here from t/node), which only tests whether the upstream in the Admin API can be created properly.

Comment thread apisix/pubsub/kafka.lua
-- string to int64 cdata numbers.
local function pb_convert_to_int64(src)
if type(src) == "string" then
return C.atoll(ffi.cast("char *", src) + 1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's check src length to avoid out of bound

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added

Comment thread docs/en/latest/pubsub.md Outdated
@@ -42,6 +42,10 @@ In Apache APISIX, the most common scenario is handling north-south traffic from

Currently, Apache APISIX supports WebSocket communication with the client, which can be any application that supports WebSocket, with Protocol Buffer as the serialization mechanism, see the [protocol definition](../../../apisix/pubsub.proto).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's update the path of definition. We should use absolute path as it's not in website

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed

Comment thread docs/en/latest/pubsub/kafka.md Outdated
title: Apache Kafka
keywords:
- APISIX
- Pub-Sub

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we use PubSub like pubsub in other places?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

All Pub-Sub replaced to PubSub


### Limitations

- Offsets need to be managed manually

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

After rendering, these two lines are merged together...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

微信截图_20220515213346

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment thread docs/en/latest/pubsub/kafka.md Outdated

### Prepare

First, it is necessary to compile the [communication protocol](../../../../apisix/pubsub.proto) as a language-specific SDK using the `protoc`, which provides the command and response definitions to connect to Kafka via APISIX using the WebSocket.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's update the path of definition. We should use absolute path as it's not in website

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed

oneof req {
CmdEmpty cmd_empty = 31;
CmdPing cmd_ping = 32;
CmdEmpty cmd_empty = 31;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can remove cmd_empty which is test-only? Using cmd_kafka_fetch in pubsub.t is enough.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@spacewander This would make the pubsub module test the relevant code that relies on kafka, and I'm not sure if I should do that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about adding a comment to show that this cmd is test-only?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After rechecking, I found that CmdEmpty has added a test-only flag.

/**
* An empty command, a placeholder for testing purposes only
*/
message CmdEmpty {}

Comment thread t/pubsub/kafka.t
@bzp2010
bzp2010 requested review from membphis and spacewander May 15, 2022 13:56
Comment thread apisix/pubsub/kafka.lua
Comment thread apisix/pubsub/kafka.lua
Comment thread apisix/pubsub/kafka.lua
Comment thread docs/en/latest/pubsub/kafka.md Outdated
@bzp2010
bzp2010 requested a review from tzssangglass May 16, 2022 07:16
tzssangglass
tzssangglass previously approved these changes May 16, 2022
@spacewander
spacewander merged commit db26b6c into apache:master May 16, 2022
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request May 20, 2022
spacewander pushed a commit that referenced this pull request Jun 30, 2022
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.

5 participants