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

Issue 7179: Adding integration tests for recovery from lts #7175

Open
wants to merge 5 commits into
base: feature_ltsrecovery
Choose a base branch
from

Conversation

kuldeepk3
Copy link
Contributor

@kuldeepk3 kuldeepk3 commented Jul 28, 2023

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

Signed-off-by: Kuldeep Kumar <Kuldeep.kumar3@dell.com>
Signed-off-by: Kuldeep Kumar <Kuldeep.kumar3@dell.com>
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

❗ No coverage uploaded for pull request base (feature_ltsrecovery@e042d47). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head ce52150 differs from pull request most recent head 85dac3e. Consider uploading reports for the commit 85dac3e to get more accurate results

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.
📢 Have feedback on the report? Share it here.

@kuldeepk3 kuldeepk3 marked this pull request as ready for review August 2, 2023 07:00
Copy link
Contributor

@sachin-j-joshi sachin-j-joshi left a 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.

readEventsFromStream(clientRunner.clientFactory, clientRunner.readerGroupManager);
log.info("Read all events again to verify that segments were recovered.");
}
FileUtils.deleteDirectory(new File(baseDir.getPath()));
Copy link
Contributor

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.
Copy link
Contributor

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.

Signed-off-by: Kuldeep Kumar <Kuldeep.kumar3@dell.com>
Signed-off-by: Kuldeep Kumar <Kuldeep.kumar3@dell.com>
Signed-off-by: Kuldeep Kumar <Kuldeep.kumar3@dell.com>
@RaulGracia RaulGracia changed the title Adding integration tests for recovery from lts Issue 7179: Adding integration tests for recovery from lts Sep 7, 2023

private FileSystemSimpleStorageFactory storageFactory;

private BookKeeperLogFactory dataLogFactory;
Copy link
Member

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;
Copy link
Member

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)
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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

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.

None yet

4 participants