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 #718] update Grpc Message Model name to SimpleMessage #723

Merged
merged 28 commits into from
Feb 10, 2022

Conversation

jinrongluo
Copy link
Contributor

@jinrongluo jinrongluo commented Jan 18, 2022

Fixes ISSUE #718

Motivation

update Grpc Message Model name to SimpleMessage

Modifications

  • sync the commits from master branch
  • update Grpc Message Model name to SimpleMessage
  • update the Grpc documentation

@jinrongluo jinrongluo changed the title [Issue #417] update Grpc Message Model name to SimpleMessage [Issue #418] update Grpc Message Model name to SimpleMessage Jan 18, 2022
@jinrongluo jinrongluo changed the title [Issue #418] update Grpc Message Model name to SimpleMessage [Issue #718] update Grpc Message Model name to SimpleMessage Jan 18, 2022
@xwm1992 xwm1992 added this to the 1.4.0 milestone Jan 19, 2022
@xwm1992
Copy link
Contributor

xwm1992 commented Feb 2, 2022

please fix the conflicts thanks. @jinrongluo

@xwm1992
Copy link
Contributor

xwm1992 commented Feb 2, 2022

If all the changes has been done, you can leave a notification and I'll do the whole test again thanks. @jinrongluo

@jinrongluo
Copy link
Contributor Author

hi @xwm1992 I still need to run more testing in myself. I will let you know when I done. thanks.

# Conflicts:
#	docs/cn/instructions/eventmesh-sdk-java-quickstart.md
#	docs/en/instructions/eventmesh-sdk-java-quickstart.md
@jinrongluo
Copy link
Contributor Author

please fix the conflicts thanks. @jinrongluo

Merge conflict is fixed.

@jinrongluo
Copy link
Contributor Author

@xwm1992 I have completed the development and testing of this PR. Please go ahead and run more testing at your end. Thanks. for the review.

@xwm1992
Copy link
Contributor

xwm1992 commented Feb 4, 2022

I run the AsyncPublishInstance and EventmeshAsyncSubscribe under the grpc module, got following error:

  • Received Server side error: INTERNAL: gRPC frame header malformed: reserved bits not zero.
    image

After got this error, the client has been shutted down and only got one message, but the topic offset has been updated to 5, this phenomenon is reasonable ?
image

this error is not inevitable, please take a look, thanks.
@jinrongluo

@jinrongluo
Copy link
Contributor Author

Hi @xwm1992 I am not able to reproduce this error in my development environment. How are you running this test?

Please make sure you use the right Publisher and Subscriber as shown below. Thanks.

image

@xwm1992
Copy link
Contributor

xwm1992 commented Feb 9, 2022

Hi, @jinrongluo if I run the AsyncPublishInstance first, then run the EventMeshAsyncSubscribe this may reproduce the error mentioned above, and for server side got the error below:

  • Receive error from client: CANCELLED: cancelled before receiving half close
  • StreamObserver Error onCompleted. CANCELLED: call already cancelled
  • (Http2ConnectionHandler.java:911) - [id: 0xdffacbb2, L:/127.0.0.1:10205 - R:/127.0.0.1:51069] Sent GOAWAY: lastStreamId '3', errorCode '2', debugData 'Stream 3 sent too many headers EOS: false'. Forcing shutdown of the connection.
    image

@jinrongluo
Copy link
Contributor Author

jinrongluo commented Feb 9, 2022

I run the AsyncPublishInstance and EventmeshAsyncSubscribe under the grpc module, got following error:

  • Received Server side error: INTERNAL: gRPC frame header malformed: reserved bits not zero.
    image

After got this error, the client has been shutted down and only got one message, but the topic offset has been updated to 5, this phenomenon is reasonable ? image

this error is not inevitable, please take a look, thanks. @jinrongluo

Thanks @xwm1992 The root cause of this issue is that Grpc StreamObserver is not thread safe. The Java doc is here:
https://grpc.github.io/grpc-java/javadoc/io/grpc/stub/StreamObserver.html

Since individual StreamObservers are not thread-safe, if multiple threads will be writing to a StreamObserver concurrently, the application must synchronize calls.

The fix is in this commit - 48ab8d3

@vongosling
Copy link
Member

I will merge to branch, we could continue review, test and bugfix. @jinrongluo @qqeasonchen

@vongosling vongosling merged commit 7332d24 into apache:grpc Feb 10, 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.

8 participants