Skip to content
This repository has been archived by the owner on Mar 31, 2023. It is now read-only.

Add grpc thread for pulsar subscribe info #274

Open
wants to merge 50 commits into
base: master
Choose a base branch
from

Conversation

luyaoluo
Copy link
Contributor

This PR contains:

  1. Add a separate gRPC thread to get pulsar subscribe info.
  2. Add a test case in gs_tests.

A sample output:
e0c51635e0815d842b3aec2e47a8b0e

luyaoluo and others added 30 commits March 10, 2021 13:11
add mq test for both multicast and unicast
Consumer now can update GoalStateV2 messages
fix pulsar producer orderingKey bugs
@luyaoluo luyaoluo requested a review from zzxgzgz January 6, 2022 02:15
@xieus xieus requested a review from lfu-ps January 6, 2022 02:16
@xieus xieus added the Feature label Jan 6, 2022
@zzxgzgz
Copy link
Contributor

zzxgzgz commented Jan 6, 2022

Hi @lly00 , thank you for the PR.

I looked at the files, and one thing I noticed is that, some of the newly added files, for example,

  • include/aca_async_grpc_subscribe_server.h
  • include/aca_grpc_subscribe.h
  • src/comm/aca_grpc_subscribe.cpp

are very similar, if not identical to the existing files.

It looks like that you are trying to add a new gRPC service to ACA. If so, you should consider utilizing the existing gRPC server/client, rather than creating new ones that serves the same purpose. With your current code, I believe it will create two gRPC servers and clients, which is not what we want.

I believe some modifications in src/comm/aca_grpc_client.cpp, src/comm/aca_grpc.cpp and their header files should be able to achieve your purpose. Could you please give it a try?

Thank you.

@luyaoluo
Copy link
Contributor Author

luyaoluo commented Jan 7, 2022

Got, I'll try it.

@xieus
Copy link
Contributor

xieus commented Mar 14, 2022

@zzxgzgz @lfu-ps Could you pls review this update, which include quite a few new commits? Thanks.

@zzxgzgz
Copy link
Contributor

zzxgzgz commented Mar 15, 2022

@lly00

I noticed that you pushed some updates recently, was the previous concerns addressed?

Also, could you please resolve the conflicts so that the checks can be run?

Thank you.

@luyaoluo
Copy link
Contributor Author

@lly00

I noticed that you pushed some updates recently, was the previous concerns addressed?

Also, could you please resolve the conflicts so that the checks can be run?

Thank you.

We added some updates to differentiate messages from gRPC and pulsar for easier debugging. I might also need to make some other updates for testing. After this, I will resolve the conflict for review.
Thanks for your comment.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants