-
Notifications
You must be signed in to change notification settings - Fork 592
HDDS-3960. Display more accurate timestamp in recon Web #1201
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
Conversation
vivekratnavel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM except for the timestamp format.
| sorter: (a: IDatanode, b: IDatanode) => a.lastHeartbeat - b.lastHeartbeat, | ||
| render: (heartbeat: number) => { | ||
| return heartbeat > 0 ? moment(heartbeat).format('lll') : 'NA'; | ||
| return heartbeat > 0 ? moment(heartbeat).format('YYYY/MM/DD HH:mm:ss') : 'NA'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use ll LTS?
| return heartbeat > 0 ? moment(heartbeat).format('YYYY/MM/DD HH:mm:ss') : 'NA'; | |
| return heartbeat > 0 ? moment(heartbeat).format('ll LTS') : 'NA'; |
| sorter: (a: IDatanode, b: IDatanode) => a.setupTime - b.setupTime, | ||
| render: (uptime: number) => { | ||
| return uptime > 0 ? moment(uptime).format('lll') : 'NA'; | ||
| return uptime > 0 ? moment(uptime).format('YYYY/MM/DD HH:mm:ss') : 'NA'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return uptime > 0 ? moment(uptime).format('YYYY/MM/DD HH:mm:ss') : 'NA'; | |
| return uptime > 0 ? moment(uptime).format('ll LTS') : 'NA'; |
| key: 'lastLeaderElection', | ||
| render: (lastLeaderElection: number) => lastLeaderElection > 0 ? | ||
| moment(lastLeaderElection).format('lll') : 'NA', | ||
| moment(lastLeaderElection).format('YYYY/MM/DD HH:mm:ss') : 'NA', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| moment(lastLeaderElection).format('YYYY/MM/DD HH:mm:ss') : 'NA', | |
| moment(lastLeaderElection).format('ll LTS') : 'NA', |
| const lastUpdatedText = lastUpdated === 0 ? 'NA' : | ||
| ( | ||
| <Tooltip | ||
| placement='bottom' title={moment(lastUpdated).format('lll')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also update the tooltip to include seconds?
| placement='bottom' title={moment(lastUpdated).format('lll')} | |
| placement='bottom' title={moment(lastUpdated).format('ll LTS')} |
|
Thanks @vivekratnavel for review. I have changed the timestamp format. |
vivekratnavel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
|
Thanks @runitao for the improvement and @vivekratnavel for the review. |

What changes were proposed in this pull request?
Display more accurate timestamp in recon Web. Currently the web only show timestamp in minutes.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3960
How was this patch tested?
Test with
pnpm run devHere is the result with this patch.
