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

Add cluster and index state checksums #8010

Merged
merged 1 commit into from Oct 15, 2014
Merged

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Oct 7, 2014

This commit adds checksumming for cluster and index states. It moves
from a plain XContent based on-disk format to a more structured binary
format including a header and footer as well as a CRC32 checksum for
each of these files. Since previous versions didn't write any format
identifier etc. this commit adds a file extension .st for states
that have header/footer and checksum.
This commit also moves over to write temporary files and moves them into
place with an atomic move operation. This commit also serializes and
deserializes the states without materilazing them into opaque memory.

Closes #7586

@s1monw s1monw added the review label Oct 7, 2014
private static final String GLOBAL_STATE_FILE_PREFIX = "global-";
private static final String INDEX_STATE_FILE_PREFIX = "state-";
private static final Pattern GLOBAL_STATE_FILE_PATTERN = Pattern.compile(GLOBAL_STATE_FILE_PREFIX + "(\\d+)(" + MetaDataStateFormat.STATE_FILE_EXTENSION + ")?");
private static final Pattern INDEX_STATE_FILE_PATTERN = Pattern.compile(INDEX_STATE_FILE_PREFIX + "(\\d+)(" + MetaDataStateFormat.STATE_FILE_EXTENSION + ")?");
Copy link
Member

Choose a reason for hiding this comment

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

Can we move these up at the top of the class with the other object variable declarations? I think it is more readable than having them 300 lines down into the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@dakrone
Copy link
Member

dakrone commented Oct 8, 2014

If possible, I think it would be useful to grab a real binary state file written in the new format and stick it in the test resources directory, then we could write a test that would read it to verify that we don't accidentally change the format of the state file written in the future. This is similar to the translog tests that read specific versions of a translog file for correctness and specific corruptions.

@s1monw
Copy link
Contributor Author

s1monw commented Oct 8, 2014

If possible, I think it would be useful to grab a real binary state file written in the new format and stick it in the test resources directory, then we could write a test that would read it to verify that we don't accidentally change the format of the state file written in the future. This is similar to the translog tests that read specific versions of a translog file for correctness and specific corruptions.

I will work on this - pushed a first round of fixes for the other comments

@s1monw
Copy link
Contributor Author

s1monw commented Oct 8, 2014

@dakrone I updated the PR and added a bunch of tests to also test reading legacy format etc. I also picked up your suggestion of testing a real binary state etc. I think we are ready here but please take the time to review this again and put all comments you have.
One thing that I am not happy with is the fact that we basically overriding files in some cases. Sometimes the general-1.st file is already there when we regenerate it which is wrong IMO. I want to introduce a generation to these files that we increment internally even if we don't bump the version. Yet, I think we should do this in a second PR

if (data.length == 0) {
logger.debug("[_global] no data for [" + stateFile.getAbsolutePath() + "], ignoring...");
logger.debug("[{}]: no data for [{}], ignoring...", stateFile.getAbsolutePath(), stateType);
Copy link
Member

Choose a reason for hiding this comment

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

is the order of the parameters to the log reversed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I fixed that in my latest commit

@s1monw
Copy link
Contributor Author

s1monw commented Oct 13, 2014

@dakrone wanna do another review on this?

@s1monw
Copy link
Contributor Author

s1monw commented Oct 13, 2014

@kimchy if you have time ^^

final List<Throwable> exceptions = new ArrayList<>();
T state = null;
CollectionUtil.timSort(files); // sort to get the highest version first
for (FileAndVersion fileAndVersion : files) {
Copy link
Member

Choose a reason for hiding this comment

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

I think here we can skip looping through all the different fileAndVersion candidates, if the first (meaning the highest version) file doesn't parse successfully, we are going to throw an exception and halt operations anyway, so we shouldn't bother continuing to read the older state files. It looks like this already breaks once one has been read anyway, so the loop isn't actually doing much other than potentially adding exceptions for older state files.

The only thing I could think of would be that maybe the latest version state file would be empty, so we should go to the next-latest version? If not, it should be replaceable with:

CollectionUtil.timSort(files); // sort to get the highest version first
if (files.size() > 0) {
  FileAndVersion fileAndVersion = files[0];
  try {
    ... parse file ...
  } catch (Throwable e) {
    throw new ElasticsearchException("failed to read state from: " + fileAndVersion.file, e);
  }
}

and skip the looping with break.

@dakrone
Copy link
Member

dakrone commented Oct 14, 2014

@s1monw gave this another review pass and left some comments.

I also tested this by starting ES, writing some data, shutting it down, corrupting the state file manually, and then starting ES back up, it spewed all kinds of errors and refused to start, so that's what I expected! :)

@s1monw
Copy link
Contributor Author

s1monw commented Oct 14, 2014

@dakrone I addressed all your comments

@dakrone
Copy link
Member

dakrone commented Oct 15, 2014

LGTM

This commit adds checksumming for cluster and index states. It moves
from a plain XContent based on-disk format to a more structured binary
format including a header and footer as well as a CRC32 checksum for
each of these files. Since previous versions didn't write any format
identifier etc. this commit adds a file extension `.st` for states
that have header/footer and checksum.
This commit also moves over to write temporary files and moves them into
place with an atomic move operation. This commit also serializes and
deserializes the states without materilazing them into opaque memory.

Closes elastic#7586
@s1monw
Copy link
Contributor Author

s1monw commented Oct 15, 2014

thx @dakrone for reviewing

@s1monw s1monw deleted the state_checksums branch October 15, 2014 09:45
@clintongormley clintongormley changed the title [CORE] Add cluster and index state checksums Add cluster and index state checksums Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checksum state files written to disk
5 participants