Skip to content

KAFKA-9891: add integration tests for EOS and StandbyTask - #8890

Merged
guozhangwang merged 1 commit into
apache:trunkfrom
mjsax:kafka-9891-currupted-state-eos-standbys
Jun 19, 2020
Merged

KAFKA-9891: add integration tests for EOS and StandbyTask#8890
guozhangwang merged 1 commit into
apache:trunkfrom
mjsax:kafka-9891-currupted-state-eos-standbys

Conversation

@mjsax

@mjsax mjsax commented Jun 17, 2020

Copy link
Copy Markdown
Member

Ports the test from #8886 to trunk -- this should be merged to 2.6 branch.

One open question. In 2.6 and trunk we rely on the active tasks to wipe out the store if it crashes. However, assume there is a hard JVM crash and we don't call closeDirty() the store would not be wiped out. Thus, I am wondering, if we would need to fix this (for both active and standby tasks) and do a check on startup if a local store must be wiped out?

The current test passes, as we do a proper cleanup after the exception is thrown.

Call for review @guozhangwang @abbccdda

@mjsax mjsax added streams tests Test fixes (including flaky tests) labels Jun 17, 2020
@ableegoldman

Copy link
Copy Markdown
Member

I thought we actually do this check on startup -- in ProcessorStateManager#initializeStoreOffsetsFromCheckpoint we throw a TaskCorruptedException if the state dir is non-empty but some offsets are missing from the checkpoint file

@guozhangwang
guozhangwang requested a review from vvcephei June 18, 2020 01:17
@guozhangwang

Copy link
Copy Markdown
Contributor

Thus, I am wondering, if we would need to fix this (for both active and standby tasks) and do a check on startup if a local store must be wiped out?

I think this is covered in trunk already? Maybe we can have a quick chat to confirm.

@mjsax

mjsax commented Jun 18, 2020

Copy link
Copy Markdown
Member Author

I looked into the code and what @ableegoldman says is correct. Did miss this before. So I guess we are good. -- I would update the ticket as "affects version 2.5.0" only.

@mjsax
mjsax force-pushed the kafka-9891-currupted-state-eos-standbys branch from 32eed31 to 386c428 Compare June 19, 2020 01:07
@guozhangwang
guozhangwang merged commit 3c43adf into apache:trunk Jun 19, 2020

@vvcephei vvcephei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this, @mjsax ! Just a couple of questions.

);

// "restart" first client and wait for standby recovery
// (could actually also be active, but it does not matter as long as we enable "state stores"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you mean "stale"?

Comment on lines +379 to +380
// need to set to zero to get predictable active/standby task assignments
streamsConfiguration.put(StreamsConfig.ACCEPTABLE_RECOVERY_LAG_CONFIG, 0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we really need this? It seems like the only thing that depends on knowing which instance would get the active is just waiting for the crash after the poison pill. Could we instead just wait for once of the instances to crash, but not worry about which?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. It's for the first phase of the test. We start the first instance and let it process the first record. As there is not enough capacity, no standby is scheduled. When we start the second instance, with "lag=0" setting, we ensure that the standby is placed at instance two. With default setting, we don't know which instance will get the active/standby assigned. -> when we inject the poison pill, we know that instance one will fail as it hosts the active.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, this was my suggestion:

Could we instead just wait for once of the instances to crash, but not worry about which?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, it make the test more complex, because all the following code depends on instance one failing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, it's your call. I think this might make the tests flaky, but I guess we can figure that out later.

@mjsax
mjsax deleted the kafka-9891-currupted-state-eos-standbys branch June 19, 2020 19:59
mjsax added a commit that referenced this pull request Jun 19, 2020
Ports the test from #8886 to trunk -- this should be merged to 2.6 branch.

One open question. In 2.6 and trunk we rely on the active tasks to wipe out the store if it crashes. However, assume there is a hard JVM crash and we don't call closeDirty() the store would not be wiped out. Thus, I am wondering, if we would need to fix this (for both active and standby tasks) and do a check on startup if a local store must be wiped out?

The current test passes, as we do a proper cleanup after the exception is thrown.

Reviewers: Guozhang Wang <wangguoz@gmail.com>
@mjsax

mjsax commented Jun 19, 2020

Copy link
Copy Markdown
Member Author

Cherry-picked to 2.6.

Kvicii pushed a commit to Kvicii/kafka that referenced this pull request Jun 21, 2020
* 'trunk' of github.com:apache/kafka:
  KAFKA-10168: fix StreamsConfig parameter name variable (apache#8865)
  MINOR: code cleanup for inconsistent naming (apache#8871)
  KAFKA-10138: Prefer --bootstrap-server for reassign_partitions command in ducktape tests (apache#8898)
  KAFKA-10185: Restoration info logging (apache#8896)
  KAFKA-9891: add integration tests for EOS and StandbyTask (apache#8890)
  MINOR: Reduce build time by gating test coverage plugins behind a flag (apache#8899)
  KAFKA-10141; Add more detail to log segment delete messages (apache#8850)
  KAFKA-10113; Specify fetch offsets correctly in `LogTruncationException` (apache#8822)
  KAFKA-10167: use the admin client to read end-offset (apache#8876)
  MINOR: Upgrade ducktape to 0.7.8 (apache#8879)
  KAFKA-10123; Fix incorrect value for AWAIT_RESET#hasPosition (apache#8841)
  KAFKA-9896: fix flaky StandbyTaskEOSIntegrationTest (apache#8883)
  MINOR: clean up unused checkstyle suppressions for Streams (apache#8861)
  MINOR: reuse toConfigObject(Map) to generate Config (apache#8889)
  MINOR: Upgrade jetty to 9.4.27.v20200227 and jersey to 2.31 (apache#8859)
  MINOR: Fix flaky HighAvailabilityTaskAssignorIntegrationTest (apache#8884)
  KAFKA-10147 MockAdminClient#describeConfigs(Collection<ConfigResource>) is unable to handle broker resource (apache#8853)
  KAFKA-10165: Remove Percentiles from e2e metrics (apache#8882)

# Conflicts:
#	core/src/main/scala/kafka/log/Log.scala
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

streams tests Test fixes (including flaky tests)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants