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

add test for compaction when exporter is slow #4377

Merged
merged 1 commit into from Apr 30, 2020
Merged

add test for compaction when exporter is slow #4377

merged 1 commit into from Apr 30, 2020

Conversation

deepthidevaki
Copy link
Contributor

Description

  • Verify that not exported events are not compacted

Related issue

closes #4371

Pull Request Checklist

  • All commit messages match our commit message guidelines
  • The submitting code follows our code style
  • If submitting code, please run mvn clean install -DskipTests locally before committing

npepinpe
npepinpe reviewed Apr 24, 2020
View reviewed changes
Copy link
Member

@npepinpe npepinpe left a comment

Choose a reason for hiding this comment

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

With the latest version of the test, do we need to write more segments once we stop exporting? Writing a few records would be enough, no? Just thinking out loud 🤔

Also, how complicated would it be to test that if we start exporting again, then we would compact properly again? 😅

@deepthidevaki
Copy link
Contributor Author

With the latest version of the test, do we need to write more segments once we stop exporting? Writing a few records would be enough, no? Just thinking out loud thinking

We have to make sure the last processed position is in the next segment. Otherwise, it will anyway not compact that segment. So we are not really testing this particular case.

Also, how complicated would it be to test that if we start exporting again, then we would compact properly again? sweat_smile

I have added that to the test. Adding a new test for that seems to be redundant. So I extended the current one.

npepinpe
npepinpe approved these changes Apr 28, 2020
View reviewed changes
Copy link
Member

@npepinpe npepinpe left a comment

Choose a reason for hiding this comment

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

👍

@npepinpe
Copy link
Member

I guess merging is blocked until we merge #4374

@npepinpe
Copy link
Member

bors r+

@npepinpe
Copy link
Member

bors r-

@zeebe-bors
Copy link
Contributor

zeebe-bors bot commented Apr 29, 2020

Canceled

@npepinpe
Copy link
Member

Sorry, thought it was already a single commit 😅

@deepthidevaki
Copy link
Contributor Author

:) I will rebase and merge.

@deepthidevaki
Copy link
Contributor Author

bors r+

zeebe-bors bot added a commit that referenced this pull request Apr 29, 2020
@zeebe-bors @deepthidevaki
Merge #4377
a688961 
4377: add test for compaction when exporter is slow r=deepthidevaki a=deepthidevaki

## Description

- Verify that not exported events are not compacted

## Related issue

closes #4371

#

Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
@zeebe-bors
Copy link
Contributor

zeebe-bors bot commented Apr 29, 2020

Build failed

@deepthidevaki
Copy link
Contributor Author

bors r+

zeebe-bors bot added a commit that referenced this pull request Apr 29, 2020
@zeebe-bors @deepthidevaki
Merge #4377
5366bec 
4377: add test for compaction when exporter is slow r=deepthidevaki a=deepthidevaki

## Description

- Verify that not exported events are not compacted

## Related issue

closes #4371

#

Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
@zeebe-bors
Copy link
Contributor

zeebe-bors bot commented Apr 29, 2020

Build failed

@deepthidevaki
Copy link
Contributor Author

bors retry

zeebe-bors bot added a commit that referenced this pull request Apr 30, 2020
@zeebe-bors @deepthidevaki
Merge #4377
64ac2b5 
4377: add test for compaction when exporter is slow r=deepthidevaki a=deepthidevaki

## Description

- Verify that not exported events are not compacted

## Related issue

closes #4371

#

Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
@zeebe-bors
Copy link
Contributor

zeebe-bors bot commented Apr 30, 2020

Build failed

@deepthidevaki
Copy link
Contributor Author

bors retry

@zeebe-bors
Copy link
Contributor

zeebe-bors bot commented Apr 30, 2020

Build succeeded

@zeebe-bors zeebe-bors bot merged commit 12ac6a5 into develop Apr 30, 2020
@zeebe-bors zeebe-bors bot deleted the 4371-exporter-compact-test branch April 30, 2020 09:19
@Zelldon
Copy link
Member

Zelldon commented Aug 4, 2020 •
edited

@deepthidevaki @npepinpe shouldn't we backport this then as well? This is actually part of a bug fix right? as we did https://github.com/zeebe-io/zeebe/pull/4363/files . I tried to backport #5106 but had to realize that I'm not able to write the same test, since there are some things missing 😅

@deepthidevaki
Copy link
Contributor Author

I don't know. At the time of this PR we were not actively backporting everything. Also this is only adding a test. On the otherhand it will be easy to backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npepinpe npepinpe npepinpe approved these changes

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

Successfully merging this pull request may close these issues.

Add integration tests to verify not exported events are not compacted
3 participants
@deepthidevaki @npepinpe @Zelldon