[ISSUE #3523] fix json serialization issue when using redis #3524

hgaol · 2023-03-23T17:44:11Z

Fixes #3523 .

Motivation

bug fix for redis storage. After this change, the json be serialized successfully and the consumer can consume the data.

Modifications

Describe the modifications you've done.

Documentation

Does this pull request introduce a new feature? (yes / no)
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

codecov · 2023-03-23T18:34:20Z

Codecov Report

Merging #3524 (cd8e857) into master (54cd00f) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

❗ Current head cd8e857 differs from pull request most recent head 3330ae7. Consider uploading reports for the commit 3330ae7 to get more accurate results

@@             Coverage Diff              @@
##             master    #3524      +/-   ##
============================================
- Coverage     14.62%   14.62%   -0.01%     
  Complexity     1526     1526              
============================================
  Files           651      651              
  Lines         31711    31716       +5     
  Branches       3033     3033              
============================================
  Hits           4638     4638              
- Misses        26657    26662       +5     
  Partials        416      416

Impacted Files	Coverage Δ
...in/java/org/apache/eventmesh/common/Constants.java	`85.71% <ø> (ø)`
...ventmesh/common/protocol/tcp/EventMeshMessage.java	`0.00% <0.00%> (ø)`
...ssage/resolver/tcp/TcpMessageProtocolResolver.java	`0.00% <0.00%> (ø)`
...me/core/protocol/grpc/service/ConsumerService.java	`0.00% <ø> (ø)`
...e/core/protocol/grpc/service/HeartbeatService.java	`0.00% <ø> (ø)`
...me/core/protocol/grpc/service/ProducerService.java	`0.00% <ø> (ø)`
...ventmesh/storage/rocketmq/admin/RocketMQAdmin.java	`0.00% <0.00%> (ø)`
...h/storage/rocketmq/admin/RocketMQAdminAdaptor.java	`0.00% <0.00%> (ø)`
...orage/standalone/admin/StandaloneAdminAdaptor.java	`0.00% <0.00%> (ø)`
...mesh/storage/standalone/admin/StandaloneAdmin.java	`92.30% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mytang0

Please pass through the header, do not write hard.

.../java/org/apache/eventmesh/protocol/meshmessage/resolver/tcp/TcpMessageProtocolResolver.java

hgaol · 2023-03-24T09:17:14Z

Please pass through the header, do not write hard.

Hi @mytang0 . Do you mean the Package header? I saw all of current headers are consistent for EventMeshMessage, see screenshot below. Or should I add another headers property with type HashMap in EventMeshMessage, and add all the headers in Package.Header?
pls let me know your preference. Thanks!

mytang0 · 2023-03-27T02:53:55Z

Please pass through the header, do not write hard.

Hi @mytang0 . Do you mean the Package header? I saw all of current headers are consistent for EventMeshMessage, see screenshot below. Or should I add another headers property with type HashMap in EventMeshMessage, and add all the headers in Package.Header? pls let me know your preference. Thanks!

Hello, that's what i thought

By default, dataContentType is 'application/json'. The problem with the test case is that the body is not json. So fixing 'text/plain' directly is problematic.
Therefore, to fundamentally solve this problem. The specified dataContentType that non-json needs to display when the producer publishes a message, and then pass the dataContentType on.
You are right to think about it. We can be simple first, directly in the header.properties

hgaol · 2023-03-27T06:14:20Z

Please pass through the header, do not write hard.

Hi @mytang0 . Do you mean the Package header? I saw all of current headers are consistent for EventMeshMessage, see screenshot below. Or should I add another headers property with type HashMap in EventMeshMessage, and add all the headers in Package.Header? pls let me know your preference. Thanks!

Hello, that's what i thought

By default, dataContentType is 'application/json'. The problem with the test case is that the body is not json. So fixing 'text/plain' directly is problematic.

Therefore, to fundamentally solve this problem. The specified dataContentType that non-json needs to display when the producer publishes a message, and then pass the dataContentType on.

You are right to think about it. We can be simple first, directly in the header.properties

updated. pls let me know if any comment. :)

hgaol · 2023-03-28T01:10:18Z

Hi @mytang0 , any comment?

eventmesh-common/src/main/java/org/apache/eventmesh/common/Constants.java

.../java/org/apache/eventmesh/protocol/meshmessage/resolver/tcp/TcpMessageProtocolResolver.java

hgaol · 2023-03-28T05:08:59Z

don't know why CI still error, I didn't change this class and it's succeeded to build on my local machine...

xwm1992 · 2023-03-28T10:00:48Z

don't know why CI still error, I didn't change this class and it's succeeded to build on my local machine...

you can find this under the HTTPClientHandler , because using try-catch with resource, you can remove the out.write in the catch block with the log.error instead.

hgaol · 2023-03-28T13:04:20Z

don't know why CI still error, I didn't change this class and it's succeeded to build on my local machine...

you can find this under the HTTPClientHandler , because using try-catch with resource, you can remove the out.write in the catch block with the log.error instead.

Thank you @xwm1992 , but seems the github action will link this PR branch to another one(maybe the master branch?). Or maybe it will pull master branch and merge the branch for this PR. Because in my branch for the PR, the HTTPClientHandler is different. You can find the code below...

https://github.com/hgaol/incubator-eventmesh/blob/3523/eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/admin/handler/HTTPClientHandler.java#L98

You can also see the newest CI which is also failed for server go, which I didn't change it... Seems the checkout action will pull the latest master branch and merge current branch, then run these actions.

To keep it simple, I'd like to create another PR to fix the CI issues.

hgaol · 2023-03-28T14:33:03Z

I've created a new PR for fixing the CI issues, see #3544 .

mytang0

LGTM

mxsm

Hi @hgaol Please rebase master to fix CI not pass

mytang0 · 2023-03-29T03:56:30Z

I've created a new PR for fixing the CI issues, see #3544 .

Already merged, please merge into your branch.

hgaol · 2023-03-29T03:59:49Z

will rebase the newest master, thank you all!

mytang0 requested changes Mar 24, 2023

View reviewed changes

mxsm reviewed Mar 24, 2023

View reviewed changes

.../java/org/apache/eventmesh/protocol/meshmessage/resolver/tcp/TcpMessageProtocolResolver.java Outdated Show resolved Hide resolved

xwm1992 requested changes Mar 28, 2023

View reviewed changes

eventmesh-common/src/main/java/org/apache/eventmesh/common/Constants.java Outdated Show resolved Hide resolved

mytang0 reviewed Mar 28, 2023

View reviewed changes

.../java/org/apache/eventmesh/protocol/meshmessage/resolver/tcp/TcpMessageProtocolResolver.java Show resolved Hide resolved

This comment was marked as duplicate.

Sign in to view

xwm1992 previously approved these changes Mar 29, 2023

View reviewed changes

xwm1992 requested a review from mytang0 March 29, 2023 01:56

mytang0 previously approved these changes Mar 29, 2023

View reviewed changes

mxsm reviewed Mar 29, 2023

View reviewed changes

hgaol added 5 commits March 29, 2023 12:00

fix json serialization issue when using redis

c4a0b6e

update for data type content

b800bdd

change data content type to lowercase and update buildEventMeshMessage

f899034

update

ef068df

update

6f1c280

hgaol dismissed stale reviews from mytang0 and xwm1992 via 6f1c280 March 29, 2023 04:01

hgaol force-pushed the 3523 branch from 9f63a3f to 6f1c280 Compare March 29, 2023 04:01

mytang0 approved these changes Mar 29, 2023

View reviewed changes

mytang0 merged commit 45825b2 into apache:master Mar 29, 2023

hgaol deleted the 3523 branch March 29, 2023 04:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE #3523] fix json serialization issue when using redis #3524

[ISSUE #3523] fix json serialization issue when using redis #3524

hgaol commented Mar 23, 2023 •

edited

Loading

codecov bot commented Mar 23, 2023 •

edited

Loading

mytang0 left a comment

hgaol commented Mar 24, 2023 •

edited

Loading

mytang0 commented Mar 27, 2023 •

edited

Loading

hgaol commented Mar 27, 2023

hgaol commented Mar 28, 2023

This comment was marked as duplicate.

hgaol commented Mar 28, 2023

xwm1992 commented Mar 28, 2023

hgaol commented Mar 28, 2023 •

edited

Loading

hgaol commented Mar 28, 2023

mytang0 left a comment

mxsm left a comment

mytang0 commented Mar 29, 2023

hgaol commented Mar 29, 2023

[ISSUE #3523] fix json serialization issue when using redis #3524

[ISSUE #3523] fix json serialization issue when using redis #3524

Conversation

hgaol commented Mar 23, 2023 • edited Loading

Motivation

Modifications

Documentation

codecov bot commented Mar 23, 2023 • edited Loading

Codecov Report

mytang0 left a comment

Choose a reason for hiding this comment

hgaol commented Mar 24, 2023 • edited Loading

mytang0 commented Mar 27, 2023 • edited Loading

hgaol commented Mar 27, 2023

hgaol commented Mar 28, 2023

This comment was marked as duplicate.

hgaol commented Mar 28, 2023

xwm1992 commented Mar 28, 2023

hgaol commented Mar 28, 2023 • edited Loading

hgaol commented Mar 28, 2023

mytang0 left a comment

Choose a reason for hiding this comment

mxsm left a comment

Choose a reason for hiding this comment

mytang0 commented Mar 29, 2023

hgaol commented Mar 29, 2023

hgaol commented Mar 23, 2023 •

edited

Loading

codecov bot commented Mar 23, 2023 •

edited

Loading

hgaol commented Mar 24, 2023 •

edited

Loading

mytang0 commented Mar 27, 2023 •

edited

Loading

hgaol commented Mar 28, 2023 •

edited

Loading