155 benchmarking by heshamMassoud · Pull Request #240 · commercetools/commercetools-sync-java

heshamMassoud · 2018-01-22T17:44:17Z

This pr addresses #155 to product benchmark tests for the library on every release. The resulting charts look like this: https://commercetools.github.io/commercetools-sync-java/benchmarks/

Benchmark Setup:

Add benchmark as a new source set in the project just like main, test and integration test.
When benchmarks are run, first checkout the gh-pages branch of the repo in a temp directory: 'tmp_git_dir/', then run the benchmarks.
Commit the results of the benchmarks (in this file: benchmarks/benchmarks.json) to this directory and push..
Use Travis' new feature of stages to have 5 different stages which runs the benchmarks and pushes the results to the gh-pages branch: https://github.com/commercetools/commercetools-sync-java/pull/240/files#diff-354f30a63fb0907d4ad57269548329e3R7
This kind of build will only be triggered on every release: if: tag IS present. This is how it looks on travis.
Add benchmark tests for ProductSync, InventorySync (still not complete and skeleton for CategorySync
In the gh-pages, the chart-render.js uses the output results of benchmarks/benchmarks.json to display the results in this bar chart graph.

Related to benchmarks:

Suppress Javadoc Checkstyle requirement for benchmark methods.
Preserve benchmarks directory on javadocs publishing.
Ensure benchmark results are only committed after the benchmarks are run.

Boy-scout-changes:

Remove unneeded oracle-java8-installer from travis configuration.
Rename gradle task from setReleaseVersion to setLibraryVersion: task | usage | in execution order script.
Rename gradle script file set-release-version to set-library-version: file itself | in build.gradle | in comment in SyncSolutionInfo

TODO:

Fix the product deletion in ITs.
Only build benchmarks on tag (remove commented out lines in travis.yml file)
Complete other missing benchmarks, but this could be done later other PRs. For now we need to merge the current benchmark setup to get results as soon as possible. (#246)
Split benchmarks.json file per version (#246)
Only show the results of the last X releases and backup the history. (#246)
Add docs/benchmarks.md to describe the benchmark setup.

andrii-kovalenko-ct · 2018-02-07T11:59:10Z

+        final List<T> list = new ArrayList<>();
+        iterator.forEachRemaining(list::add);
+        return list;
+    }


Instead of hateful stateful mutability inside closures i'd recommend to use more functional style (thought more verbose):

StreamSupport.stream(Spliterators.spliteratorUnknownSize(iterator, Spliterator.ORDERED), false) .collect(Collectors.toCollection(ArrayList::new));

with static imports looks a bit better:

stream(spliteratorUnknownSize(iterator, ORDERED), false) .collect(toCollection(ArrayList::new));

also, stream(spliteratorUnknownSize(iterator, ORDERED), false) could be carried out to util method to be reused in other methods

8ff01cd merci for the suggestion.

andrii-kovalenko-ct · 2018-02-07T12:06:20Z

+            }
+        }
+        return ofNullable(latestVersion);
+    }


return ourFancyUtilSpliteratorStreamMethod(originalRoot.fieldNames()) .reduce((first, second) -> second);

8d47b76 :)

@andrii-kovalenko-ct unneeded null check removed: a75d76c

…ot be null.

lojzatran

One more thing: I see in travis, you have things like GRGIT_USER with encrypted value. Where do we get the value if sth happens to travis? You can consider saving them as encrypted files in the repo itself, like here: https://github.com/commercetools/commercetools-paypal-plus-integration/tree/master/travis-build

lojzatran · 2018-02-11T20:00:59Z

+public class BenchmarkUtils {
+    private static final String BENCHMARK_RESULTS_FILE_NAME = "benchmarks.json";
+    private static final String BENCHMARK_RESULTS_FILE_DIR = ofNullable(System.getenv("TRAVIS_BUILD_DIR"))
+        .map(path -> path + "/tmp_git_dir/benchmarks/").orElse("");


Not sure if it's ok to pass empty String in that case. Maybe it's better to throw exception because you expect in benchmarkCommit above this structure and if it's not there, it won't work.

I need it for when I am running the benchmarks locally, I don't want an exception to be thrown. On the other hand, If this directory struc. is not there for some reason while running the benchmarks, benchmarkCommit would fail.

lojzatran · 2018-02-11T20:01:46Z

+    static final String CREATES_ONLY = "createsOnly";
+    static final String UPDATES_ONLY = "updatesOnly";
+    static final String CREATES_AND_UPDATES = "mix";
+    static double THRESHOLD = 120000; //120 seconds in milliseconds


Why not also final?

and why it's double (isn't int enough)?

andrii-kovalenko-ct · 2018-02-12T09:11:04Z

+
+public class BenchmarkUtils {
+    private static final String BENCHMARK_RESULTS_FILE_NAME = "benchmarks.json";
+    private static final String BENCHMARK_RESULTS_FILE_DIR = ofNullable(System.getenv("TRAVIS_BUILD_DIR"))


Hardcoding CI specific names (TRAVIS_BUILD_DIR) doesn't look flexible.
Better to use our own env name (for instance, BUILD_DIR, or CI_BUILD_DIR) and assign them in CI configuration

heshamMassoud · 2018-02-12T13:38:55Z

@lojzatran regarding encrypting secrets; an issue is created for improvement: #247

…hion.

codecov-io · 2018-02-13T12:06:13Z

Codecov Report

Merging #240 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #240   +/-   ##
=========================================
  Coverage     93.35%   93.35%           
  Complexity      832      832           
=========================================
  Files            69       69           
  Lines          2153     2153           
  Branches        127      127           
=========================================
  Hits           2010     2010           
  Misses          124      124           
  Partials         19       19

Impacted Files	Coverage Δ	Complexity Δ
...ercetools/sync/commons/utils/SyncSolutionInfo.java	`100% <ø> (ø)`	`1 <0> (ø)`	⬇️

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 3222700...82590a2. Read the comment docs.

heshamMassoud and others added 30 commits January 19, 2018 14:20

#155: Supply benchmark data to ITs of ProductSync and InventorySync.

86e7d23

#155: Add benchmark results table in md readfile.

efde1c7

#155: Print time taken in benchmark tests.

e0c9743

#155: Surround time with spaces.

a8702f1

#155: Make Checkstyle happy.

18703f9

#155: Suppress PMD sys.out warning.

a29532a

#155: Remove unused attribute.

4730aa1

#155: Fix Assertions in benchmarks.

1a1f60c

#155: Remove unneeded assertions.

80b1f5e

#155: Refactor benchmarks.

90a0f51

#155: Show standard streaming logs.

b866f29

#155: Fix Benchmark tests.

e1e0ea4

#155: Add travis_wait for not skipping build for long time taken.

cc9c3ec

#155: Print before assertion.

723e388

#155: Remove travis_wait

b2bda3b

Merge branch '207-improve-product-deletion' into 155-benchmark-tests

7424c5d

#155: Ignore tests to reduce build time.

c0a6bcf

#155: Remove Jacoco coverage limit.

bf6d6de

#155: Log test events in build.

8a8dad5

#155: Only run integration tests.

62d55ed

#155: Log IT events.

309ded0

#155: Log from st out only.

9f5a8c6

Create BENCHMARKS.md

24d7358

Update README.md

783d7af

Update BENCHMARKS.md

c231354

#155: Add benchmarks to release notes.

0c7df54

Update BENCHMARKS.md

ca65d42

Update BENCHMARKS.md

13bf792

Update BENCHMARKS.md

e474a64

Update BENCHMARKS.md

4521968

heshamMassoud requested review from andrii-kovalenko-ct, butenkor and lojzatran February 7, 2018 11:15

andrii-kovalenko-ct approved these changes Feb 7, 2018

View reviewed changes

heshamMassoud added 3 commits February 7, 2018 13:51

#155: Refactor iteratorToList.

8ff01cd

#155: Refactor getLatestVersionName.

8d47b76

#155: Removed nullable/flatmap because originalRoot.fieldNames() cann…

a75d76c

…ot be null.

lojzatran approved these changes Feb 11, 2018

View reviewed changes

andrii-kovalenko-ct reviewed Feb 12, 2018

View reviewed changes

#155: Make fields static and int.

e2c3ee3

heshamMassoud mentioned this pull request Feb 12, 2018

Use google cloud KMS for encrypting travis secret env vars #247

Closed

heshamMassoud and others added 12 commits February 12, 2018 14:42

#155: Use correct string.format int specifiers.

0bd4d09

#155: change name of ci build dir env var.

a21614f

#155: Add benchmarks.md to document the benchmarks setup.

5134282

#155: Fix bullet number.

c55ef4c

Update BENCHMARKS.md

773d678

Merge branch 'master' into 155-benchmark-tests

281398f

#155: Add sku to master variant data.

f7c26d5

#155: Remove unneeded benchmarks.json.

a4908e4

#155: Return back CtpQueryUtils in IT tearing down.

7271701

#155: Use consumer to complete stages on every page in a blocking fas…

7932e4b

…hion.

#155: Fix benchmarks checkstyle.

efaec0d

#155: Only run travis stages on release.

82590a2

heshamMassoud added 2 commits February 13, 2018 13:17

#155: Use correct indentation.

7024496

#155: Add release notes entry.

18d93ac

heshamMassoud merged commit 5ea07c4 into master Feb 13, 2018

heshamMassoud deleted the 155-benchmark-tests branch February 13, 2018 12:27

heshamMassoud mentioned this pull request Feb 13, 2018

Improve product sync test tearing down #207

Closed

Conversation

heshamMassoud commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark Setup:

Related to benchmarks:

Boy-scout-changes:

TODO:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heshamMassoud Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heshamMassoud Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lojzatran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heshamMassoud commented Feb 12, 2018

Uh oh!

codecov-io commented Feb 13, 2018

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

heshamMassoud commented Jan 22, 2018 •

edited

Loading

heshamMassoud Feb 7, 2018 •

edited

Loading

heshamMassoud Feb 7, 2018 •

edited

Loading