-
Notifications
You must be signed in to change notification settings - Fork 276
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.
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
Bootstrap FC optimization: Interfaces for state build and state transition flows #2946
Bootstrap FC optimization: Interfaces for state build and state transition flows #2946
Conversation
b251c8c
to
8b13123
Compare
@@ -372,6 +372,11 @@ public class ClusterMapConfig { | |||
@Default("false") | |||
public final boolean clusterMapIgnoreDownwardStateTransition; | |||
|
|||
public static final String ENABLE_FILE_COPY_FOR_BOOTSTRAP = "clustermap.enable.file.copy.for.bootstrap"; | |||
@Config(ENABLE_FILE_COPY_FOR_BOOTSTRAP) | |||
@Default("false") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge into single config
d857928
to
b4860e8
Compare
throw new StateTransitionException( | ||
"Existing Disk manager not found for replica " + partitionName + " is not found in clustermap for " + currentNode, DiskManagerNotFoundForFileCopyStateBuild); | ||
} | ||
diskManager.addBlobStoreToCompactionManager((BlobStore) store); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
store.start() and diskManager.addBlobStoreToCompactionManager((BlobStore) store) can be moved to file copy manager class later: Justin's suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate as to why would that be advantageous?
public static final String ENABLE_FILE_COPY_FOR_BOOTSTRAP = "clustermap.enable.file.copy.for.bootstrap"; | ||
@Config(ENABLE_FILE_COPY_FOR_BOOTSTRAP) | ||
@Default("false") | ||
public final boolean enableFileCopyForBootstrap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the nomenclature in this file consistent, the name of this key would be
clusterMapEnableFileCopyForBootstrap
@Config(ENABLE_FILE_COPY_FOR_BOOTSTRAP) | ||
@Default("false") | ||
public final boolean enableFileCopyForBootstrap; | ||
public static final String ENABLE_FILE_COPY_FOR_BOOTSTRAP = "clustermap.enable.file.copy.for.bootstrap"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either this or the the key defined in ClusterMapConfig needs to be removed.
Further,
- Key in this file will have
store
as prefix - Keeping the nomenclature in this file consistent, the key name would be
storeEnableFileCopyForBootstrap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted.
if (!storeConfig.enableFileCopyForBootstrap) { | ||
store.start(); | ||
} | ||
|
||
// collect store segment requirements and add into DiskSpaceAllocator | ||
List<DiskSpaceRequirements> storeRequirements = Collections.singletonList(store.getDiskSpaceRequirements()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If store.start()
hasn't been invoked, in the normal Replication protocol, would store.getDiskSpaceRequirements()
work as expected?
@@ -755,6 +760,8 @@ public void onPartitionBecomeBootstrapFromOffline(String partitionName) { | |||
} | |||
} while (!replicaAdded); | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove unnecessary new lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applicable throughout the diff wherever applicable.
throw new StateTransitionException( | ||
"Existing Disk manager not found for replica " + partitionName + " is not found in clustermap for " + currentNode, DiskManagerNotFoundForFileCopyStateBuild); | ||
} | ||
diskManager.addBlobStoreToCompactionManager((BlobStore) store); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate as to why would that be advantageous?
This PR needs to be opened with target branch |
6d54c0e
to
08af79a
Compare
08af79a
to
d3ef815
Compare
d3ef815
to
b2385bc
Compare
b2385bc
to
720c445
Compare
720c445
to
e9b1d5a
Compare
f9b9cc8
to
1d17ad8
Compare
@@ -674,6 +674,13 @@ public class StoreConfig { | |||
public final boolean storeBlockStaleBlobStoreToStart; | |||
public final static String storeBlockStaleBlobStoreToStartName = "store.block.stale.blob.store.to.start"; | |||
|
|||
|
|||
@Config(STORE_ENABLE_FILE_COPY_FOR_BOOTSTRAP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use serverReplicationProtocolForHydration
@@ -41,7 +41,8 @@ class PartitionStateChangeListenerImpl implements PartitionStateChangeListener { | |||
|
|||
@Override | |||
public void onPartitionBecomeBootstrapFromOffline(String partitionName) { | |||
|
|||
// StateBuilding at the end of FCM's Offline->Bootstrap transition | |||
storageManagerListener.buildStateForFileCopy(partitionName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to storeManager
1d17ad8
to
80d1bce
Compare
80d1bce
to
1124d46
Compare
a621b02
to
6e10bcf
Compare
…ition flows (#2946) * Changes for bootstrap improvement with statebuild * Added initial changes for state build * Changes for moving state build to FCM * Changes for state build including FCM invocation * reformatting changes --------- Co-authored-by: Jai Balani <jbalani@jbalani-mn1.linkedin.biz>
No description provided.