KAFKA-15037: pass remoteLogEnabled to unifiedLog #13779
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
showuon
commented
May 30, 2023
| @@ -159,7 +159,7 @@ public void run() { | |||
| while (!closing) { | |||
| maybeWaitForPartitionsAssignment(); | |||
|
|
|||
| log.info("Polling consumer to receive remote log metadata topic records"); | |||
| log.trace("Polling consumer to receive remote log metadata topic records"); | |||
There was a problem hiding this comment.
Logging this line on each consumer poll will flood the log. Change to trace level
|
@satishd , please take a look. Thanks. |
|
For point 2, I think @satishd has some refactoring in mind to add more logic to UnifiedLog as per #13561 (comment)
Perhaps, we can perform all the refactoring in one go together later on prior to 3.6 release. |
satishd
reviewed
May 31, 2023
| @@ -1398,7 +1402,8 @@ object LogManager { | |||
| logDirFailureChannel = logDirFailureChannel, | |||
| time = time, | |||
| keepPartitionMetadataFile = keepPartitionMetadataFile, | |||
| interBrokerProtocolVersion = config.interBrokerProtocolVersion) | |||
| interBrokerProtocolVersion = config.interBrokerProtocolVersion, | |||
| remoteStorageSystemEnable = config.getBoolean(RemoteLogManagerConfig.REMOTE_LOG_STORAGE_SYSTEM_ENABLE_PROP)) | |||
There was a problem hiding this comment.
Use config.remoteLogManagerConfig.enableRemoteStorageSystem() instead of checking the specific property in config as it is already built.
There was a problem hiding this comment.
Yes, it's better to use onfig.remoteLogManagerConfig.enableRemoteStorageSystem(). Updated.
+1 to have (2) for now. It does not need to wait for other refactorings. |
Sorry, I should have checked the build status. Fixed now. Thanks. |
|
Failed tests are unrelated (with one infra's issue): In previous build (I've rebuilt it), the failed tests are: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
UnifiedLog relied on the
remoteStorageSystemEnableto identify if the broker is enabling remote storage, but we never pass this value from the config into UnifiedLog. So it'll always be false.In this PR, I did:
remoteStorageSystemEnabletoUnifiedLogremoteLogManagerfrom the class member ofUnifiedLogsince onlyUnifiedLog#fetchOffsetByTimestampneedsremoteLogManager, and this can be passed when called fromReplicaManager.Committer Checklist (excluded from commit message)