murblanc commented on code in PR #2207:
URL: https://github.com/apache/solr/pull/2207#discussion_r1486781194
##########
solr/core/src/java/org/apache/solr/update/UpdateHandler.java:
##########
@@ -116,10 +116,13 @@ public UpdateHandler(SolrCore core, UpdateLog updateLog) {
parseEventListeners();
PluginInfo ulogPluginInfo =
core.getSolrConfig().getPluginInfo(UpdateLog.class.getName());
- // If this is a replica of type PULL, don't create the update log
+ // If this replica doesn't have a transaction log, don't create it
Review Comment:
"Doesn't require" or "doesn't use" might be clearer than "doesn't have"
because if we create the transaction log the replica will have one 😃
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2050,15 +2050,13 @@ public void reload(String name, UUID coreId) {
if (docCollection != null) {
Replica replica =
docCollection.getReplica(cd.getCloudDescriptor().getCoreNodeName());
assert replica != null : cd.getCloudDescriptor().getCoreNodeName() +
" had no replica";
- if (replica.getType() == Replica.Type.TLOG) { // TODO: needed here?
+ if (replica.getType().replicateFromLeader) {
getZkController().stopReplicationFromLeader(core.getName());
- if (!cd.getCloudDescriptor().isLeader()) {
- getZkController().startReplicationFromLeader(newCore.getName(),
true);
+ if (!replica.getType().leaderEligible ||
!cd.getCloudDescriptor().isLeader()) {
Review Comment:
If replica is not leader eligible, it can't be leader, right?
Therefore `!leaderEligible ==> !isLeader` and adding the test for
`!leaderEligible` in this `if` does not change the outcome.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java:
##########
@@ -104,27 +104,35 @@ public enum Type {
* support NRT (soft commits) and RTG. Any {@link Type#NRT} replica can
become a leader. A shard
* leader will forward updates to all active {@link Type#NRT} and {@link
Type#TLOG} replicas.
*/
- NRT(true, CollectionAdminParams.NRT_REPLICAS),
+ NRT(true, true, false, CollectionAdminParams.NRT_REPLICAS),
/**
* Writes to transaction log, but not to index, uses replication. Any
{@link Type#TLOG} replica
* can become leader (by first applying all local transaction log
elements). If a replica is of
* type {@link Type#TLOG} but is also the leader, it will behave as a
{@link Type#NRT}. A shard
* leader will forward updates to all active {@link Type#NRT} and {@link
Type#TLOG} replicas.
*/
- TLOG(true, CollectionAdminParams.TLOG_REPLICAS),
+ TLOG(true, true, true, CollectionAdminParams.TLOG_REPLICAS),
/**
* Doesn’t index or writes to transaction log. Just replicates from {@link
Type#NRT} or {@link
* Type#TLOG} replicas. {@link Type#PULL} replicas can’t become shard
leaders (i.e., if there
* are only pull replicas in the collection at some point, updates will
fail same as if there is
* no leaders, queries continue to work), so they don’t even participate
in elections.
*/
- PULL(false, CollectionAdminParams.PULL_REPLICAS);
+ PULL(false, false, true, CollectionAdminParams.PULL_REPLICAS);
public final boolean leaderEligible;
+ public final boolean requireTransactionLog;
+ public final boolean replicateFromLeader;
Review Comment:
We need Javadoc for these variables.
Regarding `replicaFromLeader`, I believe `NRT` also replicates from leader
during recovery (but not in steady state)
##########
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##########
@@ -583,7 +587,7 @@ public final void doSyncOrReplicateRecovery(SolrCore core)
throws Exception {
}
}
- if (replicaType == Replica.Type.TLOG) {
+ if (replicaType.replicateFromLeader) {
Review Comment:
In many places (this class and other classes) the test for the replica being
`TLOG` is replaced by the replica replicating from the leader (i.e. `TLOG` or
`PULL`).
Are we therefore doing for `PULL` replicas things that previously were not
done, or is the change the result of moving code execution around (but couldn't
really find things we no longer do with this PR, except the unification of
starting replication for `TLOG` and `PULL` in `ZkController` L. 1394).
##########
solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java:
##########
@@ -104,27 +104,35 @@ public enum Type {
* support NRT (soft commits) and RTG. Any {@link Type#NRT} replica can
become a leader. A shard
* leader will forward updates to all active {@link Type#NRT} and {@link
Type#TLOG} replicas.
*/
- NRT(true, CollectionAdminParams.NRT_REPLICAS),
+ NRT(true, true, false, CollectionAdminParams.NRT_REPLICAS),
/**
* Writes to transaction log, but not to index, uses replication. Any
{@link Type#TLOG} replica
* can become leader (by first applying all local transaction log
elements). If a replica is of
* type {@link Type#TLOG} but is also the leader, it will behave as a
{@link Type#NRT}. A shard
* leader will forward updates to all active {@link Type#NRT} and {@link
Type#TLOG} replicas.
*/
- TLOG(true, CollectionAdminParams.TLOG_REPLICAS),
+ TLOG(true, true, true, CollectionAdminParams.TLOG_REPLICAS),
/**
* Doesn’t index or writes to transaction log. Just replicates from {@link
Type#NRT} or {@link
* Type#TLOG} replicas. {@link Type#PULL} replicas can’t become shard
leaders (i.e., if there
* are only pull replicas in the collection at some point, updates will
fail same as if there is
* no leaders, queries continue to work), so they don’t even participate
in elections.
*/
- PULL(false, CollectionAdminParams.PULL_REPLICAS);
+ PULL(false, false, true, CollectionAdminParams.PULL_REPLICAS);
public final boolean leaderEligible;
+ public final boolean requireTransactionLog;
+ public final boolean replicateFromLeader;
public final String numReplicasPropertyName;
- Type(boolean leaderEligible, String numReplicasPropertyName) {
+ Type(
+ boolean leaderEligible,
+ boolean requireTransactionLog,
Review Comment:
It seems leaderEligible and requireTransactionLog always have the same value
(tru for TLOG and NRT, false for PULL). Do we need two parameters?
(I know ZERO replicas - see SIP-20 - in their current form might differ, but
we're not there yet)
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -2400,15 +2402,15 @@ public void rejoinShardLeaderElection(SolrParams
params) {
try (SolrCore core = cc.getCore(coreName)) {
Replica.Type replicaType =
core.getCoreDescriptor().getCloudDescriptor().getReplicaType();
- if (replicaType == Type.TLOG) {
+ if (replicaType.replicateFromLeader) {
String leaderUrl =
getLeader(
core.getCoreDescriptor().getCloudDescriptor(),
cloudConfig.getLeaderVoteWait());
if (!leaderUrl.equals(ourUrl)) {
Review Comment:
This is now checked for `PULL` replicas as well and is always `true`.
Should the `if` test the replica's `leaderEligible` flag in addition to
`replicateFromLeader`?
##########
solr/core/src/java/org/apache/solr/cloud/RecoveryStrategy.java:
##########
@@ -417,8 +419,10 @@ private final void doReplicateOnlyRecovery(SolrCore core)
throws InterruptedExce
log.error("Error while trying to recover. core={}", coreName, e);
} finally {
if (successfulRecovery) {
- log.info("Restarting background replicate from leader process");
- zkController.startReplicationFromLeader(coreName, false);
+ if (replicaType.replicateFromLeader) {
Review Comment:
Were we previously (before this PR) starting replication even for `NRT` that
does not need it?
##########
solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java:
##########
@@ -88,31 +87,9 @@ public void startReplication(boolean switchTransactionLog) {
log.info("Will start replication from leader with poll interval: {}",
pollIntervalStr);
NamedList<Object> followerConfig = new NamedList<>();
- followerConfig.add("fetchFromLeader", Boolean.TRUE);
-
- // don't commit on leader version zero for PULL replicas as PULL should
only get its index
- // state from leader
- boolean skipCommitOnLeaderVersionZero = switchTransactionLog;
Review Comment:
This seems to be the case since @HoustonPutman's change in SOLR-14679, where
the call from `ZkController.rejoinShardLeaderElection()` to
`startReplicationFromLeader()` changed its parameter from `false` to `true`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
