-
Notifications
You must be signed in to change notification settings - Fork 960
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
[RatisConsensus] Use empty map for create peer #12463
Conversation
@@ -46,6 +47,10 @@ private SchemaRegionConsensusImpl() { | |||
// do nothing | |||
} | |||
|
|||
public static boolean isRatis() { | |||
return SchemaRegionConsensusImplHolder.CONF.getSchemaRegionConsensusProtocolClass().equals(ConsensusFactory.RATIS_CONSENSUS); |
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.
ConsensusFactory.RATIS_CONSENSUS.equals(SchemaRegionConsensusImplHolder.CONF.getSchemaRegionConsensusProtocolClass())
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.
Dan man you can really write good code bro m3?
@@ -2157,7 +2157,9 @@ private TSStatus createNewRegionPeer(ConsensusGroupId regionId, List<Peer> peers | |||
if (regionId instanceof DataRegionId) { | |||
DataRegionConsensusImpl.getInstance().createLocalPeer(regionId, peers); | |||
} else { | |||
SchemaRegionConsensusImpl.getInstance().createLocalPeer(regionId, peers); | |||
final List<Peer> createParam = |
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.
I recommend not adding the isRatis function here, but instead just checking the consensus algorithm to see if an empty list should be passed in
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.
done
@@ -2157,7 +2158,12 @@ private TSStatus createNewRegionPeer(ConsensusGroupId regionId, List<Peer> peers | |||
if (regionId instanceof DataRegionId) { | |||
DataRegionConsensusImpl.getInstance().createLocalPeer(regionId, peers); |
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.
fix it too
DataRegionConsensusImpl.getInstance().createLocalPeer(regionId, peers); | ||
// RatisConsensus requires an empty peer list during region transition | ||
final List<Peer> createPeers = | ||
DataRegionConsensusImpl.getInstance() instanceof RatisConsensus |
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 config to judge
@@ -107,7 +107,7 @@ | |||
import java.util.stream.Collectors; | |||
|
|||
/** A multi-raft consensus implementation based on Apache Ratis. */ | |||
class RatisConsensus implements IConsensus { | |||
public class RatisConsensus implements IConsensus { |
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.
do not modify this
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.
LGTM
Use the entire peer list may lead to probable shutdown.