Skip to content
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.

Sign up for GitHub

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

SOLR-17579: Refactoring suggested by IDE in ReplicationHandler and tests #2893

Merged
merged 5 commits into from
Dec 14, 2024
Merged

SOLR-17579: Refactoring suggested by IDE in ReplicationHandler and tests #2893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
51c34fa
Refactoring suggested by IDE.
epugh Dec 2, 2024
982eeff
Code cleanup
epugh Dec 3, 2024
2a1a833
tidy
epugh Dec 14, 2024
02c5180
Merge remote-tracking branch 'upstream/main' into SOLR-17579
epugh Dec 14, 2024
f97b89e
Track changes
epugh Dec 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion solr/CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ Dependency Upgrades

Other Changes
---------------------
(No changes)
* SOLR-17579: Remove unused code and other refactorings in ReplicationHandler and tests. Removed unused public
Copy link
Contributor

Choose a reason for hiding this comment

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

honestly, would any reader of CHANGES.txt care? I'm not questioning your effort in this PR but ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i went back and forth on it, since I'd created a JIRA... I then thought, it should then go in Changes... Plus removing a public variable...

We do need to add to our dev docs a concise "this is what Changes" is for...

LOCAL_ACTIVITY_DURING_REPLICATION variable. (Eric Pugh)

================== 9.8.0 ==================
New Features
Expand Down
57 changes: 17 additions & 40 deletions solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,6 @@ public class IndexFetcher {

private Integer soTimeout;

private boolean downloadTlogFiles = false;

private boolean skipCommitOnLeaderVersionZero = true;

private boolean clearLocalIndexFirst = false;
Expand All @@ -209,7 +207,7 @@ public static class IndexFetchResult {
new IndexFetchResult("Local index commit is already in sync with peer", true, null);

public static final IndexFetchResult INDEX_FETCH_FAILURE =
new IndexFetchResult("Fetching lastest index is failed", false, null);
new IndexFetchResult("Fetching latest index is failed", false, null);
public static final IndexFetchResult INDEX_FETCH_SUCCESS =
new IndexFetchResult("Fetching latest index is successful", true, null);
public static final IndexFetchResult LOCK_OBTAIN_FAILED =
Expand All @@ -224,8 +222,6 @@ public static class IndexFetchResult {
public static final IndexFetchResult PEER_INDEX_COMMIT_DELETED =
new IndexFetchResult(
"No files to download because IndexCommit in peer was deleted", false, null);
public static final IndexFetchResult LOCAL_ACTIVITY_DURING_REPLICATION =
new IndexFetchResult("Local index modification during replication", false, null);
public static final IndexFetchResult EXPECTING_NON_LEADER =
new IndexFetchResult("Replicating from leader but I'm the shard leader", false, null);
public static final IndexFetchResult LEADER_IS_NOT_ACTIVE =
Expand Down Expand Up @@ -402,7 +398,7 @@ IndexFetchResult fetchLatestIndex(boolean forceReplication)
}

/**
* This command downloads all the necessary files from leader to install a index commit point.
* This command downloads all the necessary files from leader to install an index commit point.
* Only changed files are downloaded. It also downloads the conf files (if they are modified).
*
* @param forceReplication force a replication in all cases
Expand Down Expand Up @@ -670,7 +666,7 @@ IndexFetchResult fetchLatestIndex(boolean forceReplication, boolean forceCoreRel
latestGeneration);
final long timeTakenSeconds = getReplicationTimeElapsed();
final Long bytesDownloadedPerSecond =
(timeTakenSeconds != 0 ? Long.valueOf(bytesDownloaded / timeTakenSeconds) : null);
(timeTakenSeconds != 0 ? bytesDownloaded / timeTakenSeconds : null);
log.info(
"Total time taken for download (fullCopy={},bytesDownloaded={}) : {} secs ({} bytes/sec) to {}",
isFullCopyNeeded,
Expand Down Expand Up @@ -798,8 +794,7 @@ private void cleanup(
Directory indexDir,
boolean deleteTmpIdxDir,
File tmpTlogDir,
boolean successfulInstall)
throws IOException {
boolean successfulInstall) {
try {
if (!successfulInstall) {
try {
Expand Down Expand Up @@ -847,7 +842,9 @@ private void cleanup(
log.error("Error releasing indexDir", e);
}
try {
if (tmpTlogDir != null) delTree(tmpTlogDir);
if (tmpTlogDir != null) {
delTree(tmpTlogDir);
}
} catch (Exception e) {
log.error("Error deleting tmpTlogDir", e);
}
Expand Down Expand Up @@ -1368,7 +1365,7 @@ private static boolean slowFileExists(Directory dir, String fileName) throws IOE
* All the files which are common between leader and follower must have same size and same
* checksum else we assume they are not compatible (stale).
*
* @return true if the index stale and we need to download a fresh copy, false otherwise.
* @return true if the index is stale, and we need to download a fresh copy, false otherwise.
* @throws IOException if low level io error
*/
private boolean isIndexStale(Directory dir) throws IOException {
Expand Down Expand Up @@ -1516,7 +1513,7 @@ private void copyTmpConfFiles2Conf(Path tmpconfDir) {
/**
* The tlog files are moved from the tmp dir to the tlog dir as an atomic filesystem operation. A
* backup of the old directory is maintained. If the directory move fails, it will try to revert
* back the original tlog directory.
* the original tlog directory.
*/
private boolean copyTmpTlogFiles2Tlog(File tmpTlogDir) {
Path tlogDir =
Expand All @@ -1540,11 +1537,11 @@ private boolean copyTmpTlogFiles2Tlog(File tmpTlogDir) {
} catch (IOException e) {
log.error("Unable to rename: {} to: {}", src, tlogDir, e);

// In case of error, try to revert back the original tlog directory
// In case of error, try to revert the original tlog directory
try {
Files.move(backupTlogDir, tlogDir, StandardCopyOption.ATOMIC_MOVE);
} catch (IOException e2) {
// bad, we were not able to revert back the original tlog directory
// bad, we were not able to revert the original tlog directory
throw new SolrException(
SolrException.ErrorCode.SERVER_ERROR,
"Unable to rename: " + backupTlogDir + " to: " + tlogDir);
Expand Down Expand Up @@ -1598,23 +1595,6 @@ private Collection<Map<String, Object>> getModifiedConfFiles(
return nameVsFile.isEmpty() ? Collections.emptyList() : nameVsFile.values();
}

/**
* This simulates File.delete exception-wise, since this class has some strange behavior with it.
* The only difference is it returns null on success, throws SecurityException on
* SecurityException, otherwise returns Throwable preventing deletion (instead of false), for
* additional information.
*/
static Throwable delete(File file) {
try {
Files.delete(file.toPath());
return null;
} catch (SecurityException e) {
throw e;
} catch (Throwable other) {
return other;
}
}

static boolean delTree(File dir) {
try {
org.apache.lucene.util.IOUtils.rm(dir.toPath());
Expand Down Expand Up @@ -1730,8 +1710,7 @@ private class FileFetcher {
Map<String, Object> fileDetails,
String saveAs,
String solrParamOutput,
long latestGen)
throws IOException {
long latestGen) {
this.file = file;
this.fileName = (String) fileDetails.get(NAME);
this.size = (Long) fileDetails.get(SIZE);
Expand Down Expand Up @@ -1781,7 +1760,7 @@ private void fetch() throws Exception {
}
} finally {
cleanup();
// if cleanup succeeds . The file is downloaded fully. do an fsync
// if cleanup succeeds, and the file is downloaded fully, then do a fsync.
fsyncService.execute(
() -> {
try {
Expand Down Expand Up @@ -1885,8 +1864,8 @@ private int fetchPackets(FastInputStream fis) throws Exception {
}

/**
* The webcontainer flushes the data only after it fills the buffer size. So, all data has to be
* read as readFully() other wise it fails. So read everything as bytes and then extract an
* The web container flushes the data only after it fills the buffer size. So, all data has to
* be read as readFully() otherwise it fails. So read everything as bytes and then extract an
* integer out of it
*/
private int readInt(byte[] b) {
Expand Down Expand Up @@ -1953,7 +1932,7 @@ private FastInputStream getStream() throws IOException {
}
// wt=filestream this is a custom protocol
params.set(CommonParams.WT, FILE_STREAM);
// This happen if there is a failure there is a retry. the offset=<sizedownloaded> ensures
// This happens if there is a failure there is a retry. the offset=<sizedownloaded> ensures
// that the server starts from the offset
if (bytesDownloaded > 0) {
params.set(OFFSET, Long.toString(bytesDownloaded));
Expand Down Expand Up @@ -2046,16 +2025,14 @@ protected class DirectoryFileFetcher extends FileFetcher {
}

private static class LocalFsFile implements FileInterface {
private File copy2Dir;

FileChannel fileChannel;
private FileOutputStream fileOutputStream;
File file;

LocalFsFile(File dir, String saveAs) throws IOException {
this.copy2Dir = dir;

this.file = new File(copy2Dir, saveAs);
this.file = new File(dir, saveAs);

File parentDir = this.file.getParentFile();
if (!parentDir.exists()) {
Expand Down
35 changes: 10 additions & 25 deletions solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Date;
Expand Down Expand Up @@ -153,8 +152,6 @@ public class ReplicationHandler extends RequestHandlerBase
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
SolrCore core;

private volatile boolean closed = false;

@Override
public Name getPermissionName(AuthorizationContext request) {
return Name.READ_PERM;
Expand Down Expand Up @@ -227,8 +224,6 @@ public String toString() {

private volatile long executorStartTime;

private int numTimesReplicated = 0;

private final Map<String, FileInfo> confFileInfoCache = new HashMap<>();

private Long reserveCommitDuration = readIntervalMs("00:00:10");
Expand Down Expand Up @@ -323,8 +318,7 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
* @see IndexFetcher.LocalFsFileFetcher
* @see IndexFetcher.DirectoryFileFetcher
*/
private void getFileStream(SolrParams solrParams, SolrQueryResponse rsp, SolrQueryRequest req)
throws IOException {
private void getFileStream(SolrParams solrParams, SolrQueryResponse rsp, SolrQueryRequest req) {
final CoreReplication coreReplicationAPI = new CoreReplication(core, req, rsp);
String fileName;
String dirType;
Expand Down Expand Up @@ -800,14 +794,6 @@ private Date getNextScheduledExecTime() {
return nextTime;
}

int getTimesReplicatedSinceStartup() {
return numTimesReplicated;
}

void setTimesReplicatedSinceStartup() {
numTimesReplicated++;
}

@Override
public Category getCategory() {
return Category.REPLICATION;
Expand Down Expand Up @@ -1043,7 +1029,7 @@ private NamedList<Object> getReplicationDetails(
follower.add("replicationStartTime", replicationStartTimeStamp.toString());
}
long elapsed = fetcher.getReplicationTimeElapsed();
follower.add("timeElapsed", String.valueOf(elapsed) + "s");
follower.add("timeElapsed", elapsed + "s");

if (bytesDownloaded > 0)
estimatedTimeRemaining =
Expand Down Expand Up @@ -1108,13 +1094,13 @@ private Object formatVal(String key, Properties props, Class<?> clzz) {
if (s == null || s.trim().length() == 0) return null;
if (clzz == Date.class) {
try {
Long l = Long.parseLong(s);
long l = Long.parseLong(s);
return new Date(l).toString();
} catch (NumberFormatException e) {
return null;
}
} else if (clzz == List.class) {
String ss[] = s.split(",");
String[] ss = s.split(",");
List<String> l = new ArrayList<>();
for (String s1 : ss) {
l.add(new Date(Long.parseLong(s1)).toString());
Expand Down Expand Up @@ -1272,11 +1258,11 @@ public void inform(SolrCore core) {
if (enableLeader) {
includeConfFiles = (String) leader.get(CONF_FILES);
if (includeConfFiles != null && includeConfFiles.trim().length() > 0) {
List<String> files = Arrays.asList(includeConfFiles.split(","));
String[] files = includeConfFiles.split(",");
for (String file : files) {
if (file.trim().length() == 0) continue;
String[] strs = file.trim().split(":");
// if there is an alias add it or it is null
// if there is an alias add it, or it is null
confFileNameAlias.add(strs[0], strs.length > 1 ? strs[1] : null);
}
log.info("Replication enabled for following config files: {}", includeConfFiles);
Expand Down Expand Up @@ -1347,7 +1333,7 @@ public void inform(SolrCore core) {
}
}

// ensure the writer is init'd so that we have a list of commit points
// ensure the writer is initialized so that we have a list of commit points
RefCounted<IndexWriter> iw =
core.getUpdateHandler().getSolrCoreState().getIndexWriter(core);
iw.decref();
Expand Down Expand Up @@ -1532,7 +1518,8 @@ private Long readIntervalNs(String interval) {
public static final String FETCH_FROM_LEADER = "fetchFromLeader";

// in case of TLOG replica, if leaderVersion = zero, don't do commit
// otherwise updates from current tlog won't copied over properly to the new tlog, leading to data
// otherwise updates from current tlog won't be copied over properly to the new tlog, leading to
// data
// loss
// don't commit on leader version zero for PULL replicas as PULL should only get its index
Comment on lines +1522 to 1524
Copy link
Contributor

Choose a reason for hiding this comment

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

reflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dang it, I trusted gw tidy too much..

// state from leader
Expand Down Expand Up @@ -1576,8 +1563,6 @@ private Long readIntervalNs(String interval) {

public static final String ALIAS = "alias";

public static final String CONF_CHECKSUM = "confchecksum";

public static final String CONF_FILES = "confFiles";

public static final String REPLICATE_AFTER = "replicateAfter";
Expand All @@ -1601,7 +1586,7 @@ private Long readIntervalNs(String interval) {
/**
* Boolean param for tests that can be specified when using {@link #CMD_FETCH_INDEX} to force the
* current request to block until the fetch is complete. <b>NOTE:</b> This param is not advised
* for non-test code, since the duration of the fetch for non-trivial indexes will likeley cause
* for non-test code, since the duration of the fetch for non-trivial indexes will likely cause
* the request to time out.
*
* @lucene.internal
Expand Down
20 changes: 10 additions & 10 deletions solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,8 +304,8 @@ public void doTestDetails() throws Exception {
// check details on the follower a couple of times before & after fetching
for (int i = 0; i < 3; i++) {
NamedList<Object> details = getDetails(followerClient);
assertNotNull(i + ": " + details);
assertNotNull(i + ": " + details.toString(), details.get("follower"));
assertNotNull(i + ": " + details, details);
assertNotNull(i + ": " + details, details.get("follower"));

if (i > 0) {
rQuery(i, "*:*", followerClient);
Expand Down Expand Up @@ -459,7 +459,7 @@ public void doTestReplicateAfterWrite2Follower() throws Exception {
index(followerClient, "id", 555, "name", "name = " + 555);
followerClient.commit(true, true);

// this doc is added to follower so it should show an item w/ that result
// this doc is added to follower, so it should show an item w/ that result
assertEquals(1, numFound(rQuery(1, "id:555", followerClient)));

// Let's fetch the index rather than rely on the polling.
Expand Down Expand Up @@ -528,7 +528,7 @@ public void doTestIndexAndConfigReplication() throws Exception {

followerJetty.stop();

// setup an sub directory /foo/ in order to force subdir file replication
// set up a subdirectory /foo/ in order to force subdir file replication
File leaderFooDir = new File(leader.getConfDir() + File.separator + "foo");
File leaderBarFile = new File(leaderFooDir, "bar.txt");
assertTrue("could not make dir " + leaderFooDir, leaderFooDir.mkdirs());
Expand All @@ -551,7 +551,7 @@ public void doTestIndexAndConfigReplication() throws Exception {
followerQueryRsp = rQuery(1, "*:*", followerClient);
assertVersions(leaderClient, followerClient);
SolrDocument d = ((SolrDocumentList) followerQueryRsp.get("response")).get(0);
assertEquals("newname = 2000", (String) d.getFieldValue("newname"));
assertEquals("newname = 2000", d.getFieldValue("newname"));

assertTrue(followerFooDir.isDirectory());
assertTrue(followerBarFile.exists());
Expand Down Expand Up @@ -596,8 +596,8 @@ public void doTestStopPoll() throws Exception {
// get docs from leader and check if number is equal to leader
assertEquals(nDocs + 1, numFound(rQuery(nDocs + 1, "*:*", leaderClient)));

// NOTE: this test is wierd, we want to verify it DOESNT replicate...
// for now, add a sleep for this.., but the logic is wierd.
// NOTE: this test is weird, we want to verify it DOESN'T replicate...
// for now, add a sleep for this... but the logic is weird.
Thread.sleep(3000);

// get docs from follower and check if number is not equal to leader; polling is disabled
Expand Down Expand Up @@ -1583,7 +1583,7 @@ public void testEmptyBackups() throws Exception {

index(leaderClient, "id", "1", "name", "foo");

{ // second backup w/uncommited doc
{ // second backup w/uncommitted doc
final String backupName = "empty_backup2";
final GenericSolrRequest req =
new GenericSolrRequest(
Expand Down Expand Up @@ -1695,7 +1695,7 @@ private Date watchCoreStartAt(JettySolrRunner jettySolrRunner, final Date min)
return startTime;
}
} catch (SolrException e) {
// workarround for SOLR-4668
// workaround for SOLR-4668
if (500 != e.code()) {
throw e;
} // else server possibly from the core reload in progress...
Expand All @@ -1705,7 +1705,7 @@ private Date watchCoreStartAt(JettySolrRunner jettySolrRunner, final Date min)
Thread.sleep(sleepInterval);
}
fail("timed out waiting for collection1 startAt time to exceed: " + min);
return min; // compilation neccessity
return min; // compilation necessity
}
}

Expand Down
Loading
Loading