Skip to content

Comments

Refactor: Cleanup NoopTask#14938

Merged
kfaraz merged 6 commits intoapache:masterfrom
kfaraz:cleanup_nooptask
Sep 5, 2023
Merged

Refactor: Cleanup NoopTask#14938
kfaraz merged 6 commits intoapache:masterfrom
kfaraz:cleanup_nooptask

Conversation

@kfaraz
Copy link
Contributor

@kfaraz kfaraz commented Sep 4, 2023 •
edited
Loading

Changes

  • Simplify static create methods for NoopTask
  • Remove usage of FirehoseFactory and IsReadyResult from NoopTask as it was not being used anywhere
  • Update tests

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

AmatyaAvadhanula
AmatyaAvadhanula reviewed Sep 4, 2023
View reviewed changes
indexing-service/src/main/java/org/apache/druid/indexing/common/task/NoopTask.java Outdated

@VisibleForTesting
public static NoopTask create(String id, int priority)
public static NoopTask withPriority(int priority)
Copy link
Contributor

@AmatyaAvadhanula AmatyaAvadhanula Sep 4, 2023

Choose a reason for hiding this comment

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

withPriority seems a bit misleading as one may think that the other fields' values would be retained

Copy link
Contributor Author

@kfaraz kfaraz Sep 4, 2023

Choose a reason for hiding this comment

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

We could rename it to ofPriority.

AmatyaAvadhanula
AmatyaAvadhanula reviewed Sep 4, 2023
View reviewed changes
indexing-service/src/main/java/org/apache/druid/indexing/common/task/NoopTask.java
try (Firehose firehose = firehoseFactory != null ? firehoseFactory.connect(null, null) : null) {

log.info("Running noop task[%s]", getId());
log.info("Sleeping for %,d millis.", runTime);
Copy link
Contributor

@AmatyaAvadhanula AmatyaAvadhanula Sep 4, 2023

Choose a reason for hiding this comment

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

Could this logging be useful for understanding test runs better?

Copy link
Contributor Author

@kfaraz kfaraz Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, I don't see it adding much value. Such messages only fill up the logs and make it difficult to find important things such as warnings and errors.

AmatyaAvadhanula
AmatyaAvadhanula reviewed Sep 4, 2023
View reviewed changes
indexing-service/src/main/java/org/apache/druid/indexing/common/task/NoopTask.java Outdated
@JsonProperty("runTime") long runTime,
@JsonProperty("runTime") long runTimeMillis,
@JsonProperty("isReadyTime") long isReadyTime,
@JsonProperty("isReadyResult") String isReadyResult,
Copy link
Contributor

@AmatyaAvadhanula AmatyaAvadhanula Sep 4, 2023

Choose a reason for hiding this comment

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

Is a non-null value being passed for isReadyResult?

Copy link
Contributor Author

@kfaraz kfaraz Sep 4, 2023

Choose a reason for hiding this comment

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

Not really. I guess we should remove this too.

AmatyaAvadhanula
AmatyaAvadhanula approved these changes Sep 4, 2023
View reviewed changes
Copy link
Contributor

@AmatyaAvadhanula AmatyaAvadhanula left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, @kfaraz! +1 after all the checks pass

@kfaraz kfaraz merged commit 289ee1e into apache:master Sep 5, 2023
@kfaraz kfaraz deleted the cleanup_nooptask branch September 5, 2023 03:45
@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

@AmatyaAvadhanula AmatyaAvadhanula AmatyaAvadhanula approved these changes

Assignees

No one assigned

Labels

Area - Testing Refactoring

Projects

None yet

Milestone

28.0

Development

Successfully merging this pull request may close these issues.

3 participants

@kfaraz @AmatyaAvadhanula @LakshSingla