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

ISPN-8150 SingleFileStoreStressTest intermittent failure #5368

Merged
merged 1 commit into from Aug 11, 2017

Conversation

ryanemerson
Copy link
Contributor

Copy link
Member

@rvansa rvansa left a comment

Choose a reason for hiding this comment

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

Some comment (either in JIRA or in commit message) what was broken and how you've fixed id might be beneficial.

// Verify that file size decreases and the file shrinks
assertTrue(length2 <= length1);
assertTrue(length3 < length2);
assertTrue(String.format("Length2=%d, Length3=%d", length2, length3), length3 <= length2);
Copy link
Member

Choose a reason for hiding this comment

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

So the store may not shrink?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think the store is definitely not shrinking all the time. This has been failing (randomly) for a long time, see https://issues.jboss.org/browse/ISPN-5730.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the test some more, it's sleeping for 200ms, but the SingleFileStore.purge() actually does everything on the caller thread. So the test could pass a WithinThreadExecutor and skip the wait.

I also suggest testing store.getFileSize() before file.size(), just in case there's some caching going on in File.size(). I'd go even further and use TestingUtil.extractField to print the used+free entries if the store.getFileSize() check fails.

Copy link
Member

Choose a reason for hiding this comment

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

I thought this test was about making sure that it does shrink, changing the condition in here voids the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason it is not shrinking every time because the merging of entries in the first call to purge results in large FileEntries being created before the end of file. So when we attempt to write more entries, the existing FileEntries before the end of file are sufficient to accommodate the new entries and so when we call purge again, there are no free files at the end of the file to be truncated. I agree with @rvansa that my original changes void the test. I am going to push some changes that will ensure that there is always at least one free FileEntry at the end of the file and hence a truncation will occur.

@@ -323,7 +306,7 @@ private void populateStoreRandomValues(int NUM_KEYS, int MAX_VALUE_SIZE, SingleF
StreamingMarshaller marshaller, List<String> keys) {
for (int j = 0; j < NUM_KEYS; j++) {
String key = "key" + j;
String value = key + "_value_" + j + times("123456789_", new Random().nextInt(MAX_VALUE_SIZE / 10));
String value = key + "_value_" + j + times(new Random().nextInt(MAX_VALUE_SIZE / 10));
Copy link
Member

Choose a reason for hiding this comment

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

Please reformat the parameters to camelCase.

@ryanemerson ryanemerson force-pushed the ISPN-8150 branch 2 times, most recently from 257d981 to 623a6c6 Compare August 10, 2017 09:35
@@ -202,54 +200,47 @@ public void testFileTruncation() throws ExecutionException, InterruptedException
// Do some reading/writing entries with random size
final CountDownLatch stopLatch = new CountDownLatch(1);
Future[] writeFutures = new Future[NUM_WRITER_THREADS];
for (int i = 0; i < NUM_WRITER_THREADS; i++) {
for (int i = 0; i < NUM_WRITER_THREADS; i++)
Copy link
Member

Choose a reason for hiding this comment

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

-1

@ryanemerson ryanemerson force-pushed the ISPN-8150 branch 2 times, most recently from c180c3d to 6312468 Compare August 11, 2017 13:18
Caused by test logic depending on eviction order.
@danberindei danberindei merged commit 78a7868 into infinispan:master Aug 11, 2017
@danberindei
Copy link
Member

Integrated, thanks Ryan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants