Skip to content

MINOR: Improve logging for alter log dirs#6302

Merged
hachikuji merged 3 commits intoapache:trunkfrom
bob-barrett:logging
Feb 28, 2019
Merged

MINOR: Improve logging for alter log dirs#6302
hachikuji merged 3 commits intoapache:trunkfrom
bob-barrett:logging

Conversation

@bob-barrett
Copy link
Copy Markdown
Contributor

This patch adds several new log messages to provide more information about errors during log dir movement and to make it clear when each partition movement is finished.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

Thanks, left a couple comments.

// deal with partitions with errors, potentially due to leadership changes
private def handlePartitionsWithErrors(partitions: Iterable[TopicPartition]) {
if (partitions.nonEmpty)
debug(s"Handling errors for partitions $partitions")
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.

It may be helpful to add some context to this message since this method is called from several places.

} catch {
case _: KafkaStorageException =>
case e: KafkaStorageException =>
info(s"Failed to build fetch for $topicPartition with $e")
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.

I'm not sure we get much benefit from this message. It seems the only way KafkaStorageException will be raised from futureLocalReplicaOrException is if we had already caught a disk error and marked the partition offline. Perhaps debug level would be reasonable?

By the way, the exception can be passed as an argument rather than embedded in the message.

@hachikuji
Copy link
Copy Markdown
Contributor

Note I submitted #6313 which addresses another logging gap. With these two patches, at least we should be able to figure out 1) when partitions are added to the fetcher thread, 2) when the thread begins handling the partition, and 3) when the thread finishes the partition.

Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the patch.

@hachikuji hachikuji merged commit 18b3a87 into apache:trunk Feb 28, 2019
@bob-barrett bob-barrett deleted the logging branch March 4, 2019 07:46
jarekr pushed a commit to confluentinc/kafka that referenced this pull request Apr 18, 2019
* apache/trunk:
  KAFKA-7880:Naming worker thread by task id (apache#6275)
  improve some logging statements (apache#6078)
  KAFKA-7312: Change broker port used in testMinimumRequestTimeouts and testForceClose
  KAFKA-7997: Use automatic RPC generation in SaslAuthenticate
  KAFKA-8002; Log dir reassignment stalls if future replica has different segment base offset (apache#6346)
  KAFKA-3522: Add TimestampedKeyValueStore builder/runtime classes (apache#6152)
  HOTFIX: add igore import to streams_upgrade_test
  MINOR: ConsumerNetworkClient does not need to send the remaining requests when the node is not ready (apache#6264)
  KAFKA-7922: Return authorized operations in describe consumer group responses (KIP-430 Part-1)
  KAFKA-7918: Inline generic parameters Pt. III: in-memory window store (apache#6328)
  KAFKA-8012; Ensure partitionStates have not been removed before truncating. (apache#6333)
  KAFKA-8011: Fix for race condition causing concurrent modification exception (apache#6338)
  KAFKA-7912: Support concurrent access in InMemoryKeyValueStore (apache#6336)
  MINOR: Skip quota check when replica is in sync (apache#6344)
  HOTFIX: Change header back to http instead of https to path license header test (apache#6347)
  MINOR: fix release.py script (apache#6317)
  MINOR: Remove types from caching stores (apache#6331)
  MINOR: Improve logging for alter log dirs (apache#6302)
  MINOR: state.cleanup.delay.ms default is 600,000 ms (10 minutes). (apache#6345)
  MINOR: disable Streams system test for broker upgrade/downgrade (apache#6341)
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
This patch adds several new log messages to provide more information about errors during log dir movement and to make it clear when each partition movement is finished. 

Reviewers: Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants