Skip to content

Conversation

@mumrah
Copy link
Member

@mumrah mumrah commented May 18, 2021

Fixes for two compiler errors which were introduced in #10431

@mumrah mumrah requested a review from junrao May 18, 2021 20:52
Copy link
Contributor

@cmccabe cmccabe left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@mumrah : Thanks for the PR. A couple of comments below.

"Unable to close snapshot reader %s at %s",
snapshotId,
fileRecords.file
fileRecords
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to define the toString() method for FileRecords so that it prints well?

Copy link
Member

Choose a reason for hiding this comment

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

I checked. toString is defined for FileRecords. It looks like it has enough information to debug the issue.

expiredSnapshots.foreach { case (snapshotId, snapshotReader) =>
snapshotReader.foreach { reader =>
CoreUtils.swallow(reader.close(), this)
CoreUtils.swallow(reader.close(), logging)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an existing issue. But CoreUtils.swallow() doesn't seem to need logging.

@jsancio
Copy link
Member

jsancio commented May 18, 2021

LGTM. Compiles locally and KafkaMetadataLogTest passes locally. Thanks @mumrah

@mumrah mumrah merged commit 937e28d into apache:trunk May 18, 2021
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.

4 participants