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.

Sign up for GitHub

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

Jump to bottom

Format data sizes across UI #2867

Merged
merged 1 commit into from
Feb 24, 2020
Merged

Format data sizes across UI #2867

merged 1 commit into from
Feb 24, 2020

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Feb 18, 2020 •
edited by findepi
Loading

Fixes: #2810

Regression introduced in: airlift/units@f97daed#diff-9e26e64688b68bf016727ae432f099d3R209 (@JsonValue moved to other method)

@cla-bot cla-bot bot added the cla-signed label Feb 18, 2020
@wendigo wendigo requested a review from sopel39 February 18, 2020 10:55
@wendigo
Copy link
Contributor Author

wendigo commented Feb 18, 2020

Visual changes:

Zrzut ekranu 2020-02-18 o 11 41 17
Zrzut ekranu 2020-02-18 o 11 48 19
Zrzut ekranu 2020-02-18 o 11 53 57

@wendigo
Copy link
Contributor Author

wendigo commented Feb 18, 2020

Zrzut ekranu 2020-02-18 o 11 57 35

sopel39
sopel39 previously approved these changes Feb 18, 2020
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

Could you share screenshot?

presto-main/src/main/java/io/prestosql/server/DataSizeSerializer.java Outdated Show resolved Hide resolved
electrum
electrum requested changes Feb 19, 2020
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

The purpose of the DataSize change was to make the JSON representation exact. Adding this custom serializer effectively reverts that change. Instead, we should change the UI to format the values appropriately.

@wendigo wendigo force-pushed the format_ui_data_sizes branch 2 times, most recently from 9afae4c to f4e57ac Compare February 20, 2020 22:21
@wendigo
Copy link
Contributor Author

wendigo commented Feb 20, 2020

@electrum done :)

@wendigo
Copy link
Contributor Author

wendigo commented Feb 21, 2020

Rebased :)

findepi
findepi approved these changes Feb 21, 2020
View reviewed changes
presto-main/etc/hive-default-fs-site.xml Outdated Show resolved Hide resolved
presto-main/src/main/resources/webapp/src/utils.js Show resolved Hide resolved
@findepi findepi added this to the 331 milestone Feb 24, 2020
@findepi findepi merged commit 0a13a2e into trinodb:master Feb 24, 2020
@findepi
Copy link
Member

findepi commented Feb 24, 2020

Merged, thanks!

@findepi findepi mentioned this pull request Feb 24, 2020
Closed
6 tasks
@wendigo wendigo deleted the format_ui_data_sizes branch February 24, 2020 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findepi findepi findepi approved these changes

@electrum electrum electrum approved these changes

@sopel39 sopel39 sopel39 left review comments

Assignees

@electrum electrum

Labels
cla-signed
Milestone
331
Development

Successfully merging this pull request may close these issues.

UI doesn't show values in KB, MB, GB, TB
4 participants
@wendigo @findepi @electrum @sopel39