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

Jupyter notebook tutorial - Delete API #14781

Closed

Conversation

demo-kratia
Copy link
Contributor

This PR:

This PR has:

  • been self-reviewed.

Copy link
Contributor

@317brian 317brian left a comment

Choose a reason for hiding this comment

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

I haven't tested the code yet, but this is a great start.

Along with the suggestions, these are some things to watch out for:

  • Avoid using pronouns like we/our/us. Generally, you should just be directing the user, so you most of the time
  • Avoid using future tense (will) where possible

For a v1 notebook, I think it's fine thatdruidapi isn't used. That can be tackled in a subsequent PR.

Copy link
Contributor

@317brian 317brian left a comment

Choose a reason for hiding this comment

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

Some questions and nits

"id": "a4e8453e",
"metadata": {},
"source": [
"In addition to deleting by interval, you can delete segments by using `segment_id`. Let's load in some new data to work with.\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this comes after we delete the whole table? it seems inefficient to have them have to reingest a table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I grouped deleting by interval and table together since they both use the same base method, passing a time interval.

Copy link
Contributor

@317brian 317brian left a comment

Choose a reason for hiding this comment

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

Cool, the changes look great. I tested the calls, and they worked.

Just a few nits but nothing that should block merging this.

Copy link

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 17, 2024
Copy link

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants