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

Initialize translog before scheduling the sync to disk #15881

Merged
merged 2 commits into from Jan 11, 2016

Conversation

brwe
Copy link
Contributor

@brwe brwe commented Jan 11, 2016

Otherwise Translog.current might be null when we call syncNeeded().

I saw this in the log of a failed test here: http://build-us-00.elastic.co/job/es_core_21_metal/326/consoleText
Unfortunately it is not the reason for the test failure.

Otherwise Translog.current might be null when we call syncNeeded().
@@ -1480,6 +1485,23 @@ public void run() {
protected void afterAdd() throws IOException {}
}

public void testTranslogCreateWithSmallSyncInterval() throws IOException, InterruptedException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm doubting whether we should have the test - i don't think it buys much . A reference to the build failure is enough?

@bleskes
Copy link
Contributor

bleskes commented Jan 11, 2016

good catch. LGTM. Doubting about the extra test.

@s1monw
Copy link
Contributor

s1monw commented Jan 11, 2016

LGTM too I agree the test is optional but not much of a cost either

@brwe
Copy link
Contributor Author

brwe commented Jan 11, 2016

what now? remove it or not?

@s1monw
Copy link
Contributor

s1monw commented Jan 11, 2016

flip a coin

@bleskes
Copy link
Contributor

bleskes commented Jan 11, 2016

When in doubt - less is better :)

On 11 Jan 2016, at 12:26, Simon Willnauer notifications@github.com wrote:

flip a coin


Reply to this email directly or view it on GitHub.

@brwe
Copy link
Contributor Author

brwe commented Jan 11, 2016

I flipped a coin but then was too clumsy to catch it...so I tried if we have failures in unit tests without that one and there were many so I decided to remove this test.

brwe added a commit that referenced this pull request Jan 11, 2016
Initialize translog before scheduling the sync to disk
@brwe brwe merged commit 7ffccc0 into elastic:2.x Jan 11, 2016
@clintongormley clintongormley added :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. and removed :Translog labels Feb 13, 2018
@clintongormley clintongormley added :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. and removed :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. v2.0.3 v2.1.2 v2.2.0 v2.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants