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

Remove special file handling from DistributorDirectory #8276

Merged
merged 1 commit into from Nov 4, 2014

Conversation

@s1monw
Copy link
Contributor

commented Oct 29, 2014

This commit removes all special file handling from DistributorDirectory
that assigned certain files to the primary directory. This special handling
was added to ensure that files that are written more than once are essentially
overwritten. Yet this implementation is consistent all the time and doesn't need
this special handling for files that are written through this directory. Writes
to the underlying directory not going through the distributor directory are not
and have never been supported.

Note: this commit also fixes the problem of adding directories to the distributor
during restart where the primary can suddenly change and file mappings are by-passed.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2014

this is a ported version of #8275 for current master and 1.x. This entire thing came out of the fact that we write now temp files during recovery but the temp filenames will cause them to go into the wrong directories if for instance in lucene 5 a segments_N file is written to a non-primary directory.

@s1monw s1monw added the review label Oct 29, 2014
@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2014

@bleskes please take a look at this one

@bleskes
bleskes reviewed Oct 30, 2014
View changes
src/main/java/org/elasticsearch/index/store/Store.java Outdated
@@ -328,13 +336,25 @@ public static MetadataSnapshot readMetadataSnapshot(File[] indexLocations, ESLog
* verification against the checksum in the given metadata and does not add any significant overhead.
*/
public IndexOutput createVerifyingOutput(final String filename, final IOContext context, final StoreFileMetaData metadata) throws IOException {
return createVerifyingOutput(directory().createOutput(filename, context), metadata);

This comment has been minimized.

Copy link
@bleskes

bleskes Oct 30, 2014

Member

we have here the hard assumtption that the metadata describes the filename given (we use the meta data to check wether the file has legacy check sums or not). I'm worried that the can potentially go wrong as we don't bake this into the API. Also, it seems that in practice we always use metadata.name as the file name. How about changing these functions to have the following public api:

createVerifyingOutput(final StoreFileMetaData metadata, final IOContext context)

which uses directory().createOutput(metadata.name, context)

and
createVerifyingOutput(final tempFileName, final StoreFileMetaData metadata, final IOContext context)

which uses distributorDirectory.createTempOutput(tempFilename, metadata.name, IOContext context)

both call createVerifyingOutput(IndexOutput output, final StoreFileMetaData metadata) which is becomes private.

PS. it's a bit fishy that we use directory() to create an output in some codes paths and distributorDirectory in another. Potentially there are other bugs hiding here?

@bleskes
bleskes reviewed Oct 30, 2014
View changes
src/main/java/org/elasticsearch/index/store/Store.java Outdated
* Note: Checksums are calculated nevertheless since lucene does it by default sicne version 4.8.0. This method only adds the
* verification against the checksum in the given metadata and does not add any significant overhead.
*/
public IndexOutput createVerifyingOutput(IndexOutput output, final StoreFileMetaData metadata) throws IOException {
if (metadata.hasLegacyChecksum() || metadata.checksum() == null) {

This comment has been minimized.

Copy link
@bleskes

bleskes Oct 30, 2014

Member

See comment above

@bleskes
bleskes reviewed Oct 30, 2014
View changes
src/test/java/org/elasticsearch/index/store/DistributorDirectoryTest.java Outdated
dd.renameFile(new DirectoryService() {
@Override
public Directory[] build() throws IOException {
return new Directory[0]; //To change body of implemented methods use File | Settings | File Templates.

This comment has been minimized.

Copy link
@bleskes

bleskes Oct 30, 2014

Member

Remove auto generated comment?

@bleskes
bleskes reviewed Oct 30, 2014
View changes
src/test/java/org/elasticsearch/index/store/DistributorDirectoryTest.java Outdated

@Override
public long throttleTimeInNanos() {
return 0; //To change body of implemented methods use File | Settings | File Templates.

This comment has been minimized.

Copy link
@bleskes

bleskes Oct 30, 2014

Member

same here (auto gen comment)

@bleskes
bleskes reviewed Oct 30, 2014
View changes
src/test/java/org/elasticsearch/index/store/DistributorDirectoryTest.java Outdated

DistributorDirectory dd = new DistributorDirectory(distrib);
String file = RandomPicks.randomFrom(random(), Arrays.asList(Store.CHECKSUMS_PREFIX, IndexFileNames.SEGMENTS_GEN));
String tmpFileName = "tmp_" + RandomPicks.randomFrom(random(), Arrays.asList("recovery.", "foobar.", "test.")) + Math.max(0, Math.abs(random().nextLong())) + "." + file;

This comment has been minimized.

Copy link
@bleskes

bleskes Oct 30, 2014

Member

Out of curiosity - why the extra tmp_?

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 30, 2014

Author Contributor

leftover

try (IndexOutput indexOutput = status.openAndPutIndexOutput("foo.bar", new StoreFileMetaData("foo.bar", 8), status.store())) {
indexOutput.writeInt(1);
IndexOutput openIndexOutput = status.getOpenIndexOutput("foo.bar");
assertSame(openIndexOutput, indexOutput);

This comment has been minimized.

Copy link
@bleskes

bleskes Oct 30, 2014

Member

nice addition

assertNotNull(expectedFile);
status.renameAllTempFiles();
strings = Sets.newHashSet(status.store().directory().listAll());
assertTrue(strings.toString(), strings.contains("foo.bar"));

This comment has been minimized.

Copy link
@bleskes

bleskes Oct 30, 2014

Member

can we also assert that the list doesn't contain expectedFile anymore?

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 30, 2014

Author Contributor

sure - done

@bleskes

This comment has been minimized.

Copy link
Member

commented Oct 30, 2014

Left some comments

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2014

@bleskes I moved away from the actual idea and trashed the usage of the primary in the dist directory entirely except for the lock file. All the mappings from filename to directory are consistent and if you create a temp file you will only be able to rename it in the same directory which is what we want. Additionally it removes the this entire error prone situation where somebody restarts a node and adds a new data path so the primary changes. I like this much better and in the lucene 5 case it will allow us to use the Direcotry interface instead of hardcoding DistributorDirectory in Store

@bleskes
bleskes reviewed Nov 3, 2014
View changes
src/main/java/org/elasticsearch/index/store/DistributorDirectory.java Outdated
@@ -217,6 +209,9 @@ public String toString() {
* @throws IOException if the target file already exists.
*/
public void renameFile(DirectoryService directoryService, String from, String to) throws IOException {
if (IndexWriter.WRITE_LOCK_NAME.equals(from)) {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 3, 2014

Member

Do we also need to protect agains renaming to IndexWriter.WRITE_LOCK_NAME

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 3, 2014

Author Contributor

I don't think so it's a valid operation IMO we really only need to have this special mapping for some tests - I might be able to remove it but not here really.

@bleskes
bleskes reviewed Nov 3, 2014
View changes
src/main/java/org/elasticsearch/index/store/DistributorDirectory.java Outdated
@@ -75,7 +74,7 @@ public DistributorDirectory(Distributor distributor) throws IOException {
this.distributor = distributor;
for (Directory dir : distributor.all()) {
for (String file : dir.listAll()) {
if (!usePrimary(file)) {
if (IndexWriter.WRITE_LOCK_NAME.equals(file) == false) {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 3, 2014

Member

I would add a comment describing why the WRITE_LOCK has a special handling.

@bleskes
bleskes reviewed Nov 3, 2014
View changes
src/test/java/org/elasticsearch/index/store/DistributorDirectoryTest.java Outdated
dd.renameFile(service, tmpFileName, file);
try {
dd.fileLength(tmpFileName);
fail("file was renamed");

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 3, 2014

Member

can we have a better failure message - "file ["+tmpFileName + "] was renamed but still exists"

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 3, 2014

Author Contributor

done

out.writeInt(1);
}
try {
dd.renameFile(service, "foo.bar", file);

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 3, 2014

Member

do we miss a fail("") here?

This comment has been minimized.

Copy link
@s1monw

s1monw Nov 3, 2014

Author Contributor

added

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2014

@bleskes addressed your comments and moved all special casing out of this directory

@bleskes
bleskes reviewed Nov 4, 2014
View changes
src/main/java/org/elasticsearch/index/store/DistributorDirectory.java Outdated
@@ -180,22 +160,18 @@ private Directory getDirectory(String name, boolean failIfNotAssociated) throws

@Override
public Lock makeLock(String name) {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 4, 2014

Member

This method can be removed now - it's identical to the base class implementation

@bleskes
bleskes reviewed Nov 4, 2014
View changes
src/main/java/org/elasticsearch/index/store/DistributorDirectory.java Outdated
@@ -180,22 +160,18 @@ private Directory getDirectory(String name, boolean failIfNotAssociated) throws

@Override
public Lock makeLock(String name) {
return distributor.primary().makeLock(name);
return getLockFactory().makeLock(name);
}

@Override
public void clearLock(String name) throws IOException {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 4, 2014

Member

Same here - the same as base calss

@bleskes
bleskes reviewed Nov 4, 2014
View changes
src/main/java/org/elasticsearch/index/store/DistributorDirectory.java Outdated
}


public class DistributorLockFactoryWrapper extends LockFactory {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 4, 2014

Member

can we add a one lliner java doc explaining why this is needed (rather then pass through the the primary lock factory)? it's non-trivial to figure it is done to track the location of the lock file, if it's using files..

@bleskes
bleskes reviewed Nov 4, 2014
View changes
src/main/java/org/elasticsearch/index/store/DistributorDirectory.java Outdated
return distributor;
}

static boolean checkConsistency(ESLogger logger, DistributorDirectory dir) throws IOException {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 4, 2014

Member

nice addition!

@bleskes
bleskes reviewed Nov 4, 2014
View changes
src/main/java/org/elasticsearch/index/store/DistributorDirectory.java Outdated
final Directory directory = dir.nameDirMapping.get(file);
if (directory == null) {
consistent = false;
logger.info("File {} was not mapped to a directory but exists in one of the distributors directories", file);

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 4, 2014

Member

can we also assert so the failure message will include the offending file? I think it will be easier to trace that the alternative of looking for it in the logs..

@bleskes

This comment has been minimized.

Copy link
Member

commented Nov 4, 2014

@s1monw LGTM - left some minor comments but I don't think we need another review cycle. I think we should also change the description in the PR because it has become very different.

@s1monw s1monw force-pushed the s1monw:dedicated_temp_file_method branch 2 times, most recently Nov 4, 2014
@s1monw s1monw changed the title [STORE] Add dedicated method to write temporary files [STORE] Remove special file handling from DistributorDirecotry Nov 4, 2014
@s1monw s1monw force-pushed the s1monw:dedicated_temp_file_method branch Nov 4, 2014
This commit removes all special file handling from DistributorDirectory
that assigned certain files to the primary directory. This special handling
was added to ensure that files that are written more than once are essentially
overwritten. Yet this implementation is consistent all the time and doesn't need
this special handling for files that are written through this directory. Writes
to the underlying directory not going through the distributor directory are not
and have never been supported.

Note: this commit also fixes the problem of adding directories to the distributor
during restart where the primary can suddenly change and file mappings are by-passed.

Closes #8276
@s1monw s1monw force-pushed the s1monw:dedicated_temp_file_method branch to 44e24d3 Nov 4, 2014
@s1monw s1monw merged commit 44e24d3 into elastic:master Nov 4, 2014
s1monw added a commit that referenced this pull request Nov 4, 2014
This commit removes all special file handling from DistributorDirectory
that assigned certain files to the primary directory. This special handling
was added to ensure that files that are written more than once are essentially
overwritten. Yet this implementation is consistent all the time and doesn't need
this special handling for files that are written through this directory. Writes
to the underlying directory not going through the distributor directory are not
and have never been supported.

Note: this commit also fixes the problem of adding directories to the distributor
during restart where the primary can suddenly change and file mappings are by-passed.

Closes #8276
@clintongormley clintongormley changed the title [STORE] Remove special file handling from DistributorDirecotry Remove special file handling from DistributorDirecotry Jun 7, 2015
@clintongormley clintongormley changed the title Remove special file handling from DistributorDirecotry Remove special file handling from DistributorDirectory Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.