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.

Sign up for GitHub

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

Jump to bottom

fix: don't cleanup topics on engine close #4658

Merged
merged 2 commits into from
Feb 27, 2020
Merged

fix: don't cleanup topics on engine close #4658

merged 2 commits into from
Feb 27, 2020

Conversation

agavra
Copy link
Contributor

@agavra agavra commented Feb 27, 2020 •
edited
Loading

Co-authored-by: Rohan [email protected]

fixes #4654

Description

See the description in the ticket above. This makes it so that we don't cleanup topics for persistent queries on closing the engine (but we do for transient).

Testing done

  • Unit tests
  • Manual testing to ensure we can't reproduce the bug.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@agavra agavra force-pushed the fix-topic-deletion-on-exit branch 2 times, most recently from 876a543 to 7c7a222 Compare February 27, 2020 17:46
big-andy-coates
big-andy-coates reviewed Feb 27, 2020
View reviewed changes
ksql-engine/src/main/java/io/confluent/ksql/util/QueryMetadata.java Outdated
Comment on lines 183 to 185
public void stop() {
close(false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be tempted to make this abstract and move this impl to PersistentQuery.

No biggie, just feels cleaner to me.

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 wanted to do that, but there's a place where QueryMetadata is actually being instantiated directly. Not sure if that's a bug, but I didn't want to deal with it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really? Sounds like a bug!

Copy link
Contributor

@big-andy-coates big-andy-coates Feb 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like this is used from the new API code: for transient queries. So this should default to cleaning up internal topics.

Copy link
Contributor

Choose a reason for hiding this comment

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

so we should switch this to close(true) as we want transient queries to delete internal state.

This will also mean we need to remove the overloaded version from TransientQueryMetadata and add the following overload to PersistentQueryMetadata

@Override
public void stop() {
    close(false);
  }

@agavra agavra force-pushed the fix-topic-deletion-on-exit branch 3 times, most recently from de929b1 to d237f85 Compare February 27, 2020 18:11
@agavra agavra requested a review from rodesai February 27, 2020 18:15
big-andy-coates
big-andy-coates approved these changes Feb 27, 2020
View reviewed changes
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

LGTM!

@agavra @rodesai @big-andy-coates
fix: don't cleanup topics on engine close
0b52ace 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
@agavra agavra force-pushed the fix-topic-deletion-on-exit branch from d237f85 to 0b52ace Compare February 27, 2020 18:30
@agavra agavra marked this pull request as ready for review February 27, 2020 18:30
@agavra agavra requested a review from a team as a code owner February 27, 2020 18:30
@agavra agavra changed the base branch from master to 5.5.x February 27, 2020 18:30
@agavra agavra force-pushed the fix-topic-deletion-on-exit branch from 7dfb722 to f52591d Compare February 27, 2020 19:20
@agavra agavra merged commit a8ecb72 into confluentinc:5.5.x Feb 27, 2020
@agavra agavra deleted the fix-topic-deletion-on-exit branch February 27, 2020 20:06
colinhicks pushed a commit that referenced this pull request Feb 27, 2020
@agavra @rodesai
@big-andy-coates @colinhicks
fix: don't cleanup topics on engine close (#4658)
2563b46 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
colinhicks pushed a commit that referenced this pull request Feb 27, 2020
@agavra @rodesai
@big-andy-coates @colinhicks
fix: don't cleanup topics on engine close (#4658)
ad66a81 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
agavra added a commit to agavra/ksql that referenced this pull request Feb 27, 2020
@agavra @rodesai @big-andy-coates
fix: don't cleanup topics on engine close (confluentinc#4658)
b04906d 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
@agavra agavra mentioned this pull request Feb 27, 2020
Closed
@rodesai rodesai mentioned this pull request Feb 28, 2020
Closed
2 tasks
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 4, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
be9a97b 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 4, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
7b772d3 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
stevenpyzhang added a commit that referenced this pull request Mar 6, 2020
@stevenpyzhang @rodesai
@agavra @big-andy-coates
fix: backport fixes from query close (#4704)
1ac7f13 
* fix: idempotent terminate that can handle hung streams (#4643)

Fixes a couple issues with terminate:
- don't throw if the query doesn't get into NOT_RUNNING state. This can
  happen when streams threads are stuck pending shutdown.
- make terminate idempotent

* fix: don't cleanup topics on engine close (#4658)

Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>

Co-authored-by: Rohan <[email protected]>
Co-authored-by: Almog Gavra <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
@stevenpyzhang stevenpyzhang mentioned this pull request Mar 9, 2020
Merged
2 tasks
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 18, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
baa0575 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
@stevenpyzhang stevenpyzhang mentioned this pull request Mar 18, 2020
Merged
2 tasks
stevenpyzhang added a commit that referenced this pull request Mar 20, 2020
@stevenpyzhang @rodesai
@agavra @big-andy-coates
fix: backport fixes from query close (#4819)
76f960b 
* fix: idempotent terminate that can handle hung streams (#4643)

Fixes a couple issues with terminate:
- don't throw if the query doesn't get into NOT_RUNNING state. This can
  happen when streams threads are stuck pending shutdown.
- make terminate idempotent

* fix: don't cleanup topics on engine close (#4658)

Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>

Co-authored-by: Rohan <[email protected]>
Co-authored-by: Almog Gavra <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 20, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
3af0ad4 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 20, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
e7b4f7c 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 23, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
1fd3474 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 23, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
9180b52 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 24, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
f29b9eb 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 24, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
3db5df8 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
@stevenpyzhang stevenpyzhang mentioned this pull request Mar 24, 2020
Merged
2 tasks
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 24, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
6eb85a7 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
stevenpyzhang pushed a commit to stevenpyzhang/ksql that referenced this pull request Mar 24, 2020
@agavra @rodesai
@big-andy-coates @stevenpyzhang
fix: don't cleanup topics on engine close (confluentinc#4658)
3922fec 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
stevenpyzhang added a commit that referenced this pull request Mar 26, 2020
@stevenpyzhang @agavra
@rodesai @big-andy-coates
fix: don't cleanup topics on engine close (#4658) (#4857)
21e4751 
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>

Co-authored-by: Almog Gavra <[email protected]>
Co-authored-by: Rohan <[email protected]>
Co-authored-by: Andy Coates <[email protected]>
This was referenced Nov 8, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@big-andy-coates big-andy-coates big-andy-coates approved these changes

@rodesai rodesai Awaiting requested review from rodesai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

KSQL sometimes deletes internal topics when the server shuts down
2 participants
@agavra @big-andy-coates