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

Faster node initialization #1756

Merged
merged 2 commits into from
Nov 12, 2018
Merged

Faster node initialization #1756

merged 2 commits into from
Nov 12, 2018

Conversation

megakid
Copy link
Contributor

@megakid megakid commented Nov 5, 2018

Had to open a new PR for this master rebase of #1642

Cap at Environment.ProcessorCount.
Start loading biggest index files first.
@shaan1337
Copy link
Member

@megakid thanks very much! It seems that this recently added test is failing:
EventStore.Core.Tests.TransactionLog.Validation.when_validating_tfchunk_db.when_prelast_chunk_corrupted_throw_hash_validation_exception

I've done a quick test and could see the following output:

[22838,15,13:05:48.945,DEBUG] Opened completed /tmp/f0eed8a2-3af5-4cdf-a243-bd5aa20fe9c4-when_validating_tfchunk_db/chunk-000000.000000 as version 3
[22838,15,13:05:48.948,TRACE] Opened ongoing /tmp/f0eed8a2-3af5-4cdf-a243-bd5aa20fe9c4-when_validating_tfchunk_db/chunk-000001.000000 as version 3

It's strange, looks like the chunk corruption was not detected. It sounds plausible that the aggregate exception may not have been thrown properly, what do you think? (or it could be an issue with the test)

Removed `OpenOld` method that was unused.
@megakid
Copy link
Contributor Author

megakid commented Nov 10, 2018

@megakid thanks very much! It seems that this recently added test is failing:
EventStore.Core.Tests.TransactionLog.Validation.when_validating_tfchunk_db.when_prelast_chunk_corrupted_throw_hash_validation_exception

I've done a quick test and could see the following output:

[22838,15,13:05:48.945,DEBUG] Opened completed /tmp/f0eed8a2-3af5-4cdf-a243-bd5aa20fe9c4-when_validating_tfchunk_db/chunk-000000.000000 as version 3
[22838,15,13:05:48.948,TRACE] Opened ongoing /tmp/f0eed8a2-3af5-4cdf-a243-bd5aa20fe9c4-when_validating_tfchunk_db/chunk-000001.000000 as version 3

It's strange, looks like the chunk corruption was not detected. It sounds plausible that the aggregate exception may not have been thrown properly, what do you think? (or it could be an issue with the test)

Hi @shaan1337 - I found the bug which was an off-by-one error in the chunk numbering during the hash check code. This part of the method is a direct copy from the original code so not sure where the off-by-one error came in - My only explanation is that maybe this code was fixed in the master branch whilst my PR was waiting and the merge didn't pick it up as a modification.

Fixed now 👍

Copy link
Member

@shaan1337 shaan1337 left a comment

Choose a reason for hiding this comment

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

Already approved by @Lougarou after testing here: #1642

@shaan1337
Copy link
Member

@megakid thanks very much. that makes sense, it was fixed in this PR: #1653

It's strange that it didn't pick up the change after the rebase - I guess that something went wrong there. (maybe origin/master wasn't updated)
Anyway, it looks good for a merge now :) thanks very much for your contribution!

}
catch (Exception exc)
{
var msg = string.Format("Verification of chunk {0} failed, terminating server...", chunk);
Log.FatalException(exc, msg);
throw new CorruptDatabaseException(msg, exc);
Application.Exit(ExitCode.Error, msg);
Copy link
Member

Choose a reason for hiding this comment

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

I've noticed that this line has been reverted here. It's better to keep it this way. The initial plan was to make this testable by catching the exception but right now it cannot be caught from the main thread since it's thrown from a threadpool thread.

Copy link
Member

@shaan1337 shaan1337 left a comment

Choose a reason for hiding this comment

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

Did a code review, looks good 👍

@shaan1337 shaan1337 merged commit 59b71a4 into EventStore:master Nov 12, 2018
@shaan1337 shaan1337 added the area/documentation Issues relating to project documentation label Nov 12, 2018
@ChrisChinchilla ChrisChinchilla removed the area/documentation Issues relating to project documentation label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants