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.

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

Cleanup logic from handoff API #16457

Merged
merged 4 commits into from
May 16, 2024
Merged

Conversation

georgew5656
Copy link
Contributor

Some followup fixes from: #16310 (review)

Description

Making a few nit fixes from the PR in #16310 (review) for the handoff API

Key changed/added classes in this PR
  • Supervisor
  • SeekableStreamSupervisor

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.

Copy link
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 the changes, @georgew5656 .

@georgew5656
Copy link
Contributor Author

the cds-task-schema-publish-disabled test appears to be hanging and is not cancelling for some reason. i'm pretty sure this is unrelated so i'm going to merge

@georgew5656 georgew5656 merged commit ed9881d into apache:master May 16, 2024
86 of 87 checks passed
@gianm
Copy link
Contributor

gianm commented May 17, 2024

the cds-task-schema-publish-disabled test appears to be hanging and is not cancelling for some reason. i'm pretty sure this is unrelated so i'm going to merge

I've seen this happen on various PRs after master has been merged into the PR branches. It didn't happen on #16250, the PR that was merged to master immediately before this one. That suggests that possibly this patch did something to make the cds-task-schema-publish-disabled group take longer to run? It used to complete in ~1.5hrs, now it's running for 6h and then getting canceled.

@gianm
Copy link
Contributor

gianm commented May 17, 2024

Looking through the patch, though, I don't see anything that seems like it would have that effect…

@gianm
Copy link
Contributor

gianm commented May 17, 2024

Nevermind, #16250 didn't run the ITs since it was a console only change. I don't think this patch is the one that did it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants