Skip to content

[ZEPPELIN-4873]. Display rich duration info for insert into flink job#3794

Closed
zjffdu wants to merge 3 commits intoapache:masterfrom
zjffdu:ZEPPELIN-4873
Closed

[ZEPPELIN-4873]. Display rich duration info for insert into flink job#3794
zjffdu wants to merge 3 commits intoapache:masterfrom
zjffdu:ZEPPELIN-4873

Conversation

@zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Jun 10, 2020

What is this PR for?

Trivial PR which display rich duration info instead of just x seconds. See screenshot below.

What type of PR is it?

[ Improvement ]

Todos

  • - Task

What is the Jira issue?

How should this be tested?

  • CI pass

Screenshots (if appropriate)

image

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@zjffdu zjffdu force-pushed the ZEPPELIN-4873 branch 2 times, most recently from 45898c4 to 5f765eb Compare June 10, 2020 14:58

public void cancelJob(InterpreterContext context) throws InterpreterException {
LOGGER.info("Canceling job associated of paragraph: "+ context.getParagraphId());
LOGGER.info("Canceling job associated of paragraph: " + context.getParagraphId());
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to use {} placeholder instead of concatenation - it won't lead to the call of the context.getParagraphId() if logger is not configured for INFO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}

static String toRichTimeDuration(long duration) {
long days = TimeUnit.SECONDS.toDays(duration);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just use Duration data type, and then use corresponding functions, like, toDays, toHours, ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems using Duration doesn't' save work compared current approach, I still have to get days and minus it from duration, and the same for hours, minutes, seconds. So I didn't change it.

}
}

static String toRichTimeDuration(long duration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

would be useful to add a javadoc for it, saying what is parameter is - seconds, milliseconds, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

javadoc is added

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

lgtm

@asfgit asfgit closed this in 3139ed6 Jun 16, 2020
asfgit pushed a commit that referenced this pull request Jun 16, 2020
Trivial PR which display rich duration info instead of just x seconds. See screenshot below.

[ Improvement ]

* [ ] - Task

* https://issues.apache.org/jira/browse/ZEPPELIN-4873

* CI pass

![image](https://user-images.githubusercontent.com/164491/84286308-19291b80-ab71-11ea-96ef-b237d2463b8c.png)

* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jeff Zhang <zjffdu@apache.org>

Closes #3794 from zjffdu/ZEPPELIN-4873 and squashes the following commits:

459f181 [Jeff Zhang] add java doc
419818e [Jeff Zhang] address comment
9691eae [Jeff Zhang] [ZEPPELIN-4873]. Display rich duration info for insert into flink job

(cherry picked from commit 3139ed6)
Signed-off-by: Jeff Zhang <zjffdu@apache.org>
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.

2 participants