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.

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

Cut over to Path API #8611

Merged
merged 1 commit into from Nov 25, 2014

Conversation

Projects
None yet
4 participants
@s1monw
Copy link
Contributor

commented Nov 22, 2014

This commit also refactors RAFReference to work on top of a Channel instead of fetching the channel from the RandomAccessFile. The channel is created to truncate the file it opens too. This likely needs some documentation or we should allow the caller to pass the open options but I wanted to get this change out for review.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2014

@dakrone I assigned you here but I'd love @rmuir to look at this too...

@dakrone

View changes

src/main/java/org/elasticsearch/index/gateway/local/LocalIndexShardGateway.java Outdated
Path recoveringTranslogFile = null;
for (Path translogLocation : translog.locations()) {
Path tmpRecoveringFile = translogLocation.resolve(recoverTranslogName);
if (Files.exists(tmpRecoveringFile) == false) {

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

This can be Files.notExist(...)

@@ -219,7 +224,7 @@ public void recover(boolean indexShouldExists, RecoveryState recoveryState) thro
}
}

if (recoveringTranslogFile == null || !recoveringTranslogFile.exists()) {
if (recoveringTranslogFile == null || Files.exists(recoveringTranslogFile) == false) {

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

Same here, Files.notExists(...)

@dakrone

View changes

src/main/java/org/elasticsearch/index/translog/TranslogStreams.java Outdated
try (InputStreamStreamInput headerStream = new InputStreamStreamInput(new FileInputStream(translogFile));) {
if (translogFile.exists() == false || translogFile.length() == 0) {
try (InputStreamStreamInput headerStream = new InputStreamStreamInput(Files.newInputStream(translogFile));) {
if (Files.exists(translogFile) == false || Files.size(translogFile) == 0) {

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

Another for Files.notExists(...)

@dakrone

View changes

src/main/java/org/elasticsearch/index/translog/fs/ChannelReference.java Outdated
public boolean increaseRefCount() {
return refCount.incrementAndGet() > 1;
public void deleteOnClose() {
deleteOnClose = true;

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

I think it would be better to pass a boolean in to this method, since it's ambiguous from the name of the method whether it sets a var (could be named setDeleteOnClose() if it were setting something) or actually does the deleting.

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 24, 2014

Author Contributor

why would you want this, it only means you can unset the boolean which sucks.

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

Because deleteOnClose() is an immediate verb, I would be fine with renaming it setDeleteOnClose() if that sounds any better

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

It would also allow you to change the

if (delete) {
  channel.deleteOnClose();
}
channel.close();

to

channel.deleteOnClose(delete);
channel.close();
@dakrone

View changes

src/main/java/org/elasticsearch/index/translog/fs/FsTranslog.java Outdated
long currentFree = file.getFreeSpace();
Path location = null;
for (Path file : locations) {
long currentFree = Files.getFileStore(file).getUsableSpace();

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

Extra space between "=" and "Files" here (not a big deal)

@dakrone

View changes

src/main/java/org/elasticsearch/index/translog/fs/FsTranslog.java Outdated
for (File file : files) {
if (file.getName().equals("translog-" + current.id())) {
for (Path location : locations) {
try (DirectoryStream<Path> stream = Files.newDirectoryStream(location)) {

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

I think the "translog-" string should be a public global static var named TRANSLOG_FILENAME_PREFIX, and should be used as the glob pattern for the directory stream. Then all the hardcoded "translog-" strings in here can use the PREFIX var (I did this the hard way in my version of the branch where I typo'd the prefix and it took forever to debug, so having a static prefix to use would be nice).

This PREFIX can also be used in LocalIndexShardGateway.recover since it's hardcoded to "translog-" + id there too.

@dakrone

This comment has been minimized.

Copy link
Member

commented Nov 24, 2014

@s1monw left a couple of comments but other than that it looks good!

@dakrone

This comment has been minimized.

Copy link
Member

commented Nov 24, 2014

LGTM

@dakrone

This comment has been minimized.

Copy link
Member

commented Nov 24, 2014

LGTM (still :) )

@rmuir

View changes

src/main/java/org/elasticsearch/index/gateway/local/LocalIndexShardGateway.java Outdated
@@ -219,7 +225,7 @@ public void recover(boolean indexShouldExists, RecoveryState recoveryState) thro
}
}

if (recoveringTranslogFile == null || !recoveringTranslogFile.exists()) {
if (recoveringTranslogFile == null || Files.notExists(recoveringTranslogFile)) {

This comment has been minimized.

Copy link
@rmuir

rmuir Nov 24, 2014

Contributor

Are we sure this is the correct logic? this logic means, if the file "definitely does not exist". But i think we want !Files.exists()? Unless the file "definitely exists"

@rmuir

View changes

src/main/java/org/elasticsearch/index/translog/TranslogStreams.java Outdated

try (InputStreamStreamInput headerStream = new InputStreamStreamInput(new FileInputStream(translogFile));) {
if (translogFile.exists() == false || translogFile.length() == 0) {
try (InputStreamStreamInput headerStream = new InputStreamStreamInput(Files.newInputStream(translogFile));) {

This comment has been minimized.

Copy link
@rmuir

rmuir Nov 24, 2014

Contributor

Is this semicolon really meant to be here?

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 24, 2014

Author Contributor

not really I am removing

@s1monw s1monw force-pushed the s1monw:cutover_translog_path_api branch Nov 24, 2014

@rmuir

View changes

src/main/java/org/elasticsearch/index/translog/fs/ChannelReference.java Outdated

private final FileChannel channel;

private final AtomicInteger refCount = new AtomicInteger();
private volatile boolean deleteOnClose = false;

This comment has been minimized.

Copy link
@rmuir

rmuir Nov 24, 2014

Contributor

does this really have to be a volatile mutable property? Shouldn't we know up front, for any channel, if we want to delete on close? What uses this delete on close and can we avoid it?

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 24, 2014

Author Contributor

this was used before I just refactored this I am happy to revisit this API but this is just a cutover though...

This comment has been minimized.

Copy link
@rmuir

rmuir Nov 24, 2014

Contributor

I only mentioned it because if we really have to keep this, then StandardOpenOption.DELETE_ON_CLOSE could be an implementation. But this one has race conditions too, this delete-on-close stuff is why Lucene's lockfactories were buggy for years. Lets defer it to a new issue, ideally we just nuke it completely.

@s1monw s1monw force-pushed the s1monw:cutover_translog_path_api branch Nov 24, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2014

@dakrone I had to do another round of unfucking here sorry - can you take another look?

@dakrone

View changes

src/main/java/org/elasticsearch/index/gateway/local/LocalIndexShardGateway.java Outdated
@@ -285,7 +291,7 @@ public void recover(boolean indexShouldExists, RecoveryState recoveryState) thro
}
} catch (Throwable e) {
// we failed to recovery, make sure to delete the translog file (and keep the recovering one)
indexShard.translog().closeWithDelete();
IOUtils.closeWhileHandlingException(indexShard.translog());

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

Can you remove the now-misleading comment above this line? The original translog file should not exist because we moved it to the .recovering version

@dakrone

View changes

src/main/java/org/elasticsearch/index/translog/fs/ChannelReference.java Outdated
private final AtomicInteger refCount = new AtomicInteger();

public RafReference(File file) throws FileNotFoundException {
public ChannelReference(Path file, boolean reuseExisting) throws IOException {

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

I definitely think it's worth adding an explanation of reuseExisting to the javadocs for this constructor, when I see "reuseExisting" I don't expect it to truncate an existing file, so we should be very careful with documenting this.

@rmuir

View changes

src/main/java/org/elasticsearch/index/gateway/local/LocalIndexShardGateway.java Outdated
String recoverTranslogName = translogName + ".recovering";
Translog translog = indexShard.translog();
final Path translogName = translog.getPath(translogId);
final Path recoverTranslogName = Paths.get(translogName + ".recovering");

This comment has been minimized.

Copy link
@rmuir

rmuir Nov 24, 2014

Contributor

Can we please avoid Paths.get here? We should always minimize its usage since it makes things hard to test, etc.

I would do something like:

translogName.resolveSibling(translogName.getFileName + ".recovering");
@dakrone

View changes

src/main/java/org/elasticsearch/index/translog/fs/FsTranslog.java Outdated
@@ -288,8 +266,17 @@ public void newTransientTranslog(long id) throws TranslogException {
}
}

private Path getCurrentPath() {

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

This method is unused, should it be removed?

@dakrone

View changes

src/test/java/org/elasticsearch/index/translog/AbstractSimpleTranslogTests.java Outdated
assertFileDeleted(translog, translog.currentId()-1);
}
assertFileIsPresent(translog, translog.currentId());
IOUtils.rm(locations); // delete all the locations
super.tearDown();

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

super.tearDown() should be called in a finally block

@dakrone

View changes

src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
indexWriter.setCommitData(Collections.singletonMap(Translog.TRANSLOG_ID_KEY, Long.toString(translogIdGenerator.get())));
indexWriter.commit();
}
translog.newTranslog(translogIdGenerator.get());
;

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

extra ; here

@dakrone

View changes

src/main/java/org/elasticsearch/index/translog/fs/FsTranslog.java Outdated
try {
if (isReferencedTranslogFile(file()) == false) {
// if the given path is not the current we can safely delete the file since all references are released
logger.debug("delete translog file - not referenced and not current anymore " + file());

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

Should use {} logging style instead of string concatenation here

try {
sync();
} finally {
channelReference.decRef();

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

I wonder if we should extend ChannelReference to implement Closeable and have the .close() method decrement the ref and throw an error if it's not now at 0 (which it should be since it's being closed in this case). That way we error immediately if the translog is closed but something incremented the channel ref.

What do you think of this? Dunno if you think it might make ChannelReference too complex.

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

Hmm... I'm not actually sure how this would play with Snapshots, so it may be a no-go.

@dakrone

View changes

src/main/java/org/elasticsearch/indices/InternalIndicesService.java Outdated
}
}));

logger.debug("[{}] closing index service", index, reason);

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

I just realized, for this log message and all of the ones below it here, we only log index and totally omit reason because there is only one {} in the log message...

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 24, 2014

Author Contributor

fixed

@dakrone

View changes

src/test/java/org/elasticsearch/index/translog/AbstractSimpleTranslogTests.java Outdated
translog.newTranslog(2);
assertThat(translog.currentId(), Matchers.not(equalTo(firstId)));
translog.newTranslog(2);
assertThat(translog.currentId(), Matchers.not(equalTo(firstId)));

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

Random indentation?

@dakrone

View changes

src/test/java/org/elasticsearch/index/translog/AbstractSimpleTranslogTests.java Outdated
assertThat(firstSnapshot.estimatedTotalOperations(), equalTo(1));
translog.clearUnreferenced();
if (randomBoolean()) {
translog.clearUnreferenced();

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

Does the second clearUnreferenced check that it doesn't throw exceptions here?

@dakrone

View changes

src/test/java/org/elasticsearch/index/translog/AbstractSimpleTranslogTests.java Outdated

public void assertFileIsPresent(Translog translog, long id) {
for (Path location : translog.locations()) {
assertTrue(Files.exists(location.resolve(translog.getPath(id))));

This comment has been minimized.

Copy link
@dakrone

dakrone Nov 24, 2014

Member

Wouldn't this fail with multiple data paths since we only write the translog file in one location and this checks that it's written in all locations?

@dakrone

This comment has been minimized.

Copy link
Member

commented Nov 25, 2014

LGTM

[TRANSLOG] Cut over to Path API
This commit moves all the Translog related code over to the
NIO2 Path API. It also make transaction logs write once since it
never reuses a translog file.

Closes #8611

@s1monw s1monw force-pushed the s1monw:cutover_translog_path_api branch to 35b278f Nov 25, 2014

@s1monw s1monw merged commit 35b278f into elastic:master Nov 25, 2014

@s1monw s1monw deleted the s1monw:cutover_translog_path_api branch Nov 25, 2014

@s1monw s1monw removed the v1.5.0 label Nov 25, 2014

@clintongormley clintongormley removed the review label Mar 19, 2015

@clintongormley clintongormley changed the title [TRANSLOG] Cut over to Path API Cut over to Path API Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.