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

[dashboard] fix peek parse message error #4918

Merged
merged 1 commit into from
Aug 14, 2019

Conversation

yittg
Copy link
Contributor

@yittg yittg commented Aug 8, 2019

Fixes #4917

Motivation

dashboard peek message api raise exception when format response cause treat all message as a JSON string

Modifications

  • format as JSON only if message is a JSON,
  • otherwise if message is printable, return the original message,
  • otherwise print hex like command with --hex option.

@yittg yittg force-pushed the fix-dashboard-peek branch 4 times, most recently from 801e1a5 to 409e63f Compare August 9, 2019 02:28
@yittg yittg changed the title [WIP] fix dashboard peek parse message error [dashboard] fix peek parse message error Aug 9, 2019
@yittg
Copy link
Contributor Author

yittg commented Aug 9, 2019

cc @sijie @rshermanTHG

@yittg yittg force-pushed the fix-dashboard-peek branch 4 times, most recently from 393db26 to a2b6ec2 Compare August 9, 2019 03:27
@rshermanTHG
Copy link
Contributor

rshermanTHG commented Aug 9, 2019

From a functionality point of view looks good to me. Can't really comment on how Pythonic the code is as I only hack Python.

@yittg
Copy link
Contributor Author

yittg commented Aug 9, 2019

take a closer look about the api implementation, found that this is a wrong deserialize implementation, it didn't distinguish the batch mode or not.

sorry for that, just hold it.

@yittg yittg changed the title [dashboard] fix peek parse message error [WIP][dashboard] fix peek parse message error Aug 9, 2019
@yittg
Copy link
Contributor Author

yittg commented Aug 10, 2019

For now, we can not parse batch messages payload without metadata pb struct except those with size of 1. So in this PR, only parse rightly for normal and 1-batch message, and display the size only of other batch messages.

It can be resolved in another PR for those batch messages if need.

@sijie what do you think?

@sijie
Copy link
Member

sijie commented Aug 10, 2019

@yittg sgtm

@yittg
Copy link
Contributor Author

yittg commented Aug 10, 2019

@sijie yeah, let's do it, PTAL. by the way, here's some screenshots:
image
image
image

@yittg yittg changed the title [WIP][dashboard] fix peek parse message error [dashboard] fix peek parse message error Aug 10, 2019
@yittg
Copy link
Contributor Author

yittg commented Aug 10, 2019

run cpp tests
run integration tests

@sijie sijie added this to the 2.4.1 milestone Aug 13, 2019
@sijie
Copy link
Member

sijie commented Aug 13, 2019

@tuteng can you please help review this?

@tuteng
Copy link
Member

tuteng commented Aug 13, 2019

lgtm

@sijie sijie merged commit a8b57c9 into apache:master Aug 14, 2019
@yittg yittg deleted the fix-dashboard-peek branch August 14, 2019 01:36
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.

[dashboard] peek parse response raise exception
4 participants