-
Notifications
You must be signed in to change notification settings - Fork 404
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
Issue 7179: Adding integration tests for recovery from lts #7175
base: feature_ltsrecovery
Are you sure you want to change the base?
Issue 7179: Adding integration tests for recovery from lts #7175
Conversation
Signed-off-by: Kuldeep Kumar <Kuldeep.kumar3@dell.com>
Signed-off-by: Kuldeep Kumar <Kuldeep.kumar3@dell.com>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## feature_ltsrecovery #7175 +/- ##
======================================================
Coverage ? 86.19%
Complexity ? 16130
======================================================
Files ? 1035
Lines ? 60201
Branches ? 6114
======================================================
Hits ? 51892
Misses ? 5115
Partials ? 3194 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some quick comments.
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Outdated
Show resolved
Hide resolved
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Outdated
Show resolved
Hide resolved
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Outdated
Show resolved
Hide resolved
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Show resolved
Hide resolved
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Outdated
Show resolved
Hide resolved
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Show resolved
Hide resolved
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Outdated
Show resolved
Hide resolved
readEventsFromStream(clientRunner.clientFactory, clientRunner.readerGroupManager); | ||
log.info("Read all events again to verify that segments were recovered."); | ||
} | ||
FileUtils.deleteDirectory(new File(baseDir.getPath())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if exception is thrown?
log.info("Started segment store and controller again."); | ||
|
||
log.info("Starting client to read Events "); | ||
// Create the client with new controller. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like there is opportunity to refactor common code.
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Kuldeep Kumar <Kuldeep.kumar3@dell.com>
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Outdated
Show resolved
Hide resolved
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Outdated
Show resolved
Hide resolved
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Outdated
Show resolved
Hide resolved
test/integration/src/test/java/io/pravega/test/integration/BackUpRecoveryTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Kuldeep Kumar <Kuldeep.kumar3@dell.com>
Signed-off-by: Kuldeep Kumar <Kuldeep.kumar3@dell.com>
|
||
private FileSystemSimpleStorageFactory storageFactory; | ||
|
||
private BookKeeperLogFactory dataLogFactory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be initialized by each test individually. It can be made into a local.
|
||
@Override | ||
protected int getThreadPoolSize() { | ||
return 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why so high?
* 7. Reads all events. | ||
* @throws Exception In case of an exception occurred while execution. | ||
*/ | ||
@Test(timeout = 180000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and elsewhere in this file:
This is going to run on every single checkin. Right now we run 1000+ tests in ~20 minutes. If these are taking anywhere near three minutes each, they need to be speed up.
pravegaRunner.close(); | ||
log.info("SegmentStore, BookKeeper & ZooKeeper shutdown"); | ||
|
||
Thread.sleep(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sleeping is not allowed in unit tests.
pravegaRunner.close(); | ||
log.info("SegmentStore, BookKeeper & ZooKeeper shutdown"); | ||
|
||
Thread.sleep(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sleeping is not allowed in unit tests
Signed-off-by: Kuldeep Kumar Kuldeep.kumar3@dell.com
Change log description
Adding integration tests for recovery from lts
Purpose of the change
Fixes #7179
What the code does
Adds integration tests
How to verify it
NA