Skip to content

Improve logging in CoordinatorBasedSegmentHandoffNotifier#14640

Merged
kfaraz merged 5 commits intoapache:masterfrom
aho135:fix-time-unit-in-CoordinatorBasedSegmentHandoffNotifier
Jul 24, 2023
Merged

Improve logging in CoordinatorBasedSegmentHandoffNotifier#14640
kfaraz merged 5 commits intoapache:masterfrom
aho135:fix-time-unit-in-CoordinatorBasedSegmentHandoffNotifier

Conversation

@aho135
Copy link
Copy Markdown
Contributor

@aho135 aho135 commented Jul 21, 2023 •
edited
Loading

Description

Fixes time unit for Segment Handoff log from seconds to milliseconds

Changed secs to ms for time unit in CoordinatorBasedSegmentHandoffNotifier

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Release note

Key changed/added classes in this PR

CoordinatorBasedSegmentHandoffNotifier

This PR has:

  • been self-reviewed.
  • a release note entry in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • been tested in a test Druid cluster.

abhishekrb19
abhishekrb19 reviewed Jul 21, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@abhishekrb19 abhishekrb19 left a comment

Choose a reason for hiding this comment

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

Good catch, @aho135. Thank you for the contribution!

Minor suggestions - ISO 8601 duration is a bit friendlier to read than raw milliseconds

Comment thread ...r/src/main/java/org/apache/druid/segment/handoff/CoordinatorBasedSegmentHandoffNotifier.java Outdated
log.error(
e,
"Exception while checking handoff for dataSource[%s] Segment[%s], Will try again after [%d]secs",
"Exception while checking handoff for dataSource[%s] Segment[%s], Will try again after [%d]ms",
Copy link
Copy Markdown
Contributor

@abhishekrb19 abhishekrb19 Jul 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
"Exception while checking handoff for dataSource[%s] Segment[%s], Will try again after [%d]ms",
"Exception while checking handoff for datasource[%s] segment[%s]; will try again after [%s]",

Comment thread ...r/src/main/java/org/apache/druid/segment/handoff/CoordinatorBasedSegmentHandoffNotifier.java Outdated
"Exception while checking handoff for dataSource[%s] Segment[%s], Will try again after [%d]ms",
dataSource,
descriptor,
pollDurationMillis
Copy link
Copy Markdown
Contributor

@abhishekrb19 abhishekrb19 Jul 21, 2023

Choose a reason for hiding this comment

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

Suggested change
pollDurationMillis
new Duration(pollDurationMillis).toString()

Comment thread ...r/src/main/java/org/apache/druid/segment/handoff/CoordinatorBasedSegmentHandoffNotifier.java Outdated
log.error(
t,
"Exception while checking handoff for dataSource[%s], Will try again after [%d]secs",
"Exception while checking handoff for dataSource[%s], Will try again after [%d]ms",
Copy link
Copy Markdown
Contributor

@abhishekrb19 abhishekrb19 Jul 21, 2023

Choose a reason for hiding this comment

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

Suggested change
"Exception while checking handoff for dataSource[%s], Will try again after [%d]ms",
"Exception while checking handoff for datasource[%s]; will try again after [%s]",

Copy link
Copy Markdown
Contributor Author

@aho135 aho135 Jul 21, 2023

Choose a reason for hiding this comment

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

Thanks for the review @abhishekrb19 Just pushed an update

abhishekrb19
abhishekrb19 approved these changes Jul 21, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@abhishekrb19 abhishekrb19 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

kfaraz
kfaraz reviewed Jul 22, 2023
View reviewed changes
Comment thread ...r/src/main/java/org/apache/druid/segment/handoff/CoordinatorBasedSegmentHandoffNotifier.java Outdated
dataSource,
descriptor,
pollDurationMillis
Duration.ofMillis(pollDurationMillis).toString()
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Jul 22, 2023

Choose a reason for hiding this comment

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

We might as well just change the field long pollDurationMillis to be Duration pollDuration instead. The constructor already takes a Duration. We could also use ScheduledExecutors.scheduleAtFixedRate in line 75 for a Duration friendly invocation.

kfaraz
kfaraz reviewed Jul 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

Minor comment, otherwise looks good.

Comment thread ...r/src/main/java/org/apache/druid/segment/handoff/CoordinatorBasedSegmentHandoffNotifier.java Outdated
dataSource,
descriptor,
pollDurationMillis
pollDuration.toString()
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Jul 23, 2023

Choose a reason for hiding this comment

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

You can just use pollDuration instead of pollDuration.toString() here as the .toString() is implicit.

Comment thread ...r/src/main/java/org/apache/druid/segment/handoff/CoordinatorBasedSegmentHandoffNotifier.java Outdated
"Exception while checking handoff for dataSource[%s]; will try again after [%s]",
dataSource,
pollDurationMillis
pollDuration.toString()
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Jul 23, 2023

Choose a reason for hiding this comment

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

Same comment here.

kfaraz
kfaraz approved these changes Jul 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

Thanks for your first contribution, @aho135 ! 🚀

kfaraz
kfaraz reviewed Jul 23, 2023
View reviewed changes
Comment thread ...r/src/main/java/org/apache/druid/segment/handoff/CoordinatorBasedSegmentHandoffNotifier.java
Comment on lines 29 to +30

import org.joda.time.Duration;
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Jul 23, 2023

Choose a reason for hiding this comment

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

Suggested change
import org.joda.time.Duration;
import org.joda.time.Duration;

Copy link
Copy Markdown
Contributor

@kfaraz kfaraz Jul 23, 2023

Choose a reason for hiding this comment

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

this is causing checkstyle to fail.

Copy link
Copy Markdown
Contributor Author

@aho135 aho135 Jul 23, 2023

Choose a reason for hiding this comment

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

Thanks for the review @kfaraz! Just pushed this fix

@kfaraz kfaraz merged commit 607f511 into apache:master Jul 24, 2023
@kfaraz kfaraz changed the title Fix time unit for handoff in CoordinatorBasedSegmentHandoffNotifier Improve logging in CoordinatorBasedSegmentHandoffNotifier Jul 24, 2023
@LakshSingla LakshSingla added this to the 28.0 milestone Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfaraz kfaraz kfaraz approved these changes

@abhishekrb19 abhishekrb19 abhishekrb19 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

28.0

Development

Successfully merging this pull request may close these issues.

4 participants

@aho135 @abhishekrb19 @kfaraz @LakshSingla