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

Upgrade gradle, fix test logging and int test CPU usage #1235

Merged
merged 1 commit into from
Aug 14, 2019

Conversation

cgtz
Copy link
Contributor

@cgtz cgtz commented Aug 13, 2019

  • Upgrade to gradle 5.2.1. As part of this, I have separated the
    integration tests into their own task so that they can be run
    independently. The "allTest" target will do what the "test" target
    used to do.
  • Suppress some especially noisy logs in integration tests. These log
    messages were firing continuously when the node-down scenarios were
    tested.
  • Configure MockCluster replication threads to do a 100 ms sleep per
    iteration. This greatly reduces the CPU requirements for running the
    integration tests and allows us to run them on travis-ci.
  • Set up travis to run integration tests too.

@jsjtzyy
Copy link
Contributor

jsjtzyy commented Aug 13, 2019

We can close #1031 once current PR is merged.

@codecov-io
Copy link

codecov-io commented Aug 13, 2019

Codecov Report

Merging #1235 into master will increase coverage by 5.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1235      +/-   ##
============================================
+ Coverage     83.62%   88.63%   +5.01%     
- Complexity       57       61       +4     
============================================
  Files             6        6              
  Lines           348      352       +4     
  Branches         38       37       -1     
============================================
+ Hits            291      312      +21     
+ Misses           45       29      -16     
+ Partials         12       11       -1
Impacted Files Coverage Δ Complexity Δ
.../com/github/ambry/account/HelixAccountService.java 87.59% <0%> (+1.37%) 39% <0%> (+2%) ⬆️
...thub/ambry/account/HelixAccountServiceFactory.java 87.5% <0%> (+58.33%) 3% <0%> (+2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db6d337...6c99703. Read the comment docs.

@cgtz cgtz requested review from jsjtzyy and lightningrob August 13, 2019 16:06
Copy link
Contributor

@jsjtzyy jsjtzyy left a comment

Choose a reason for hiding this comment

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

Looks good. Minor and optional comments (can be addressed in future PR)

@@ -793,9 +796,6 @@ private void getEntriesSinceTest(NavigableMap<MockId, NavigableSet<IndexValue>>
highestIdIncluded = referenceIndex.lastKey().equals(idToCheck) ? null : referenceIndex.lastKey();
// check the case where maxSize is more than the number of entries in the segment
doGetEntriesSinceTest(referenceIndex, segment, idToCheck, maxSize + 1, 0, highestIdIncluded);
Copy link
Contributor

@jsjtzyy jsjtzyy Aug 13, 2019

Choose a reason for hiding this comment

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

One thing we can improve in the future is doGetEntriesSinceTest is invoked multiple times and some are in the loop. I find that doGetEntriesSinceTest test both getEntriesSince() and getIndexEntriesSince() methods. Note that getIndexEntriesSince() with oneEntryPerKey = true is actually tested in getEntriesSince(). It seems we don't have to explicitly test oneEntryPerKey = true for getIndexEntriesSince() again. (My two cents.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I will leave the test since whether or not it is called is dependent on the implementation of getEntriesSince and getIndexEntriesSince is still exposed to the package. I'll think about how to further reduce the test length in future PRs.

.travis.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@lightningrob lightningrob left a comment

Choose a reason for hiding this comment

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

LGTM

- Upgrade to gradle 5.2.1. As part of this, I have separated the
  integration tests into their own task so that they can be run
  independently. The "allTest" target will do what the "test" target
  used to do.
- Suppress some especially noisy logs in integration tests. These log
  messages were firing continuously when the node-down scenarios were
  tested.
- Configure MockCluster replication threads to do a 100 ms sleep per
  iteration. This greatly reduces the CPU requirements for running the
  integration tests and allows us to run them on travis-ci.
- Set up travis to run integration tests too.
@cgtz cgtz force-pushed the gradle-test-fixes branch from cbac5c4 to 6c99703 Compare August 14, 2019 17:14
@jsjtzyy jsjtzyy merged commit 62d752a into linkedin:master Aug 14, 2019
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.

4 participants