Migrations-1015 - Add buildscript for replayer #118

okhasawn · 2023-03-09T21:40:16Z

Description

This adds a gradle build script file to make it possible to repeatedly build the Traffic Replayer, compiling all java files, run unit tests, checkstyle and spotbugs on the code, all with one command.

You can either run gradle build within the traffic replayer directory, or to do this with a gradle wrapper, then you must run gradle wrapper then run ./gradlew build

Issues Resolved

Migrations-1015

Check List

New functionality includes testing
- All tests pass, including unit test, integration test and doctest
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

codecov-commenter · 2023-03-09T21:42:00Z

Codecov Report

Merging #118 (ff51474) into main (f792138) will increase coverage by 91.51%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##           main     #118       +/-   ##
=========================================
+ Coverage      0   91.51%   +91.51%     
=========================================
  Files         0       25       +25     
  Lines         0     1155     +1155     
  Branches      0      122      +122     
=========================================
+ Hits          0     1057     +1057     
- Misses        0       83       +83     
- Partials      0       15       +15

see 25 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

gregschohn · 2023-03-09T22:57:16Z

Can you look up what the conventions are for other opensearch gradle projects. What are some of the target names, are there any other specific tools that are commonly used (like checkstyle)? You can create more tasks to add them, but it's good to maintain consistency for future developers.

okhasawn · 2023-03-09T23:43:58Z

Can you look up what the conventions are for other opensearch gradle projects. What are some of the target names, are there any other specific tools that are commonly used (like checkstyle)? You can create more tasks to add them, but it's good to maintain consistency for future developers.

opensearch-project/opensearch-java repo actually has checkstyle as a github action, while the opensearch-project/opensearch stopped using checkstyle and started using spotless

Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

…ommit fixes that Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

gregschohn

I'm not familiar w/ a lot of the specific details that need to go into the build.gradle file, but I've left some comments. I'd like to make sure that this works well with IDE integration and the command line. Please add some notes to the PR with your findings.

gregschohn · 2023-03-20T15:25:49Z

TrafficReplayer/build.gradle

+    implementation group: 'com.beust', name: 'jcommander', version: '1.82'
+    implementation group: 'org.apache.logging.log4j', name: 'log4j-api', version: '2.20.0'
+    annotationProcessor 'org.projectlombok:lombok:1.18.10'


Thank you for noticing & adding these!

gregschohn · 2023-03-20T15:29:32Z

TrafficReplayer/build.gradle

+sourceSets {
+    main {
+        java {
+            srcDirs = ['src/org/opensearch/migrations/replay','src/org/opensearch/migrations/replay/netty']


I would expect the source set to just be 'src' since org... are all part of the same source hierarchy. This might create an issue when importing this into an IDE. If you just make this 'src', does the project still build? Does it load well into IntelliJ (I don't know how vscode works, but if you know, that's a good datapoint to try too)?

Thanks for letting me know and pointing this out. It does still build and load well into IntelliJ, I'll update this and "tst" too!

gregschohn · 2023-03-20T15:29:43Z

TrafficReplayer/build.gradle

+        java {
+            compileClasspath += main.output + test.output
+            runtimeClasspath += main.output + test.output
+            resources.srcDir file('tst/org/opensearch/migrations/replay')


Same as above - should this just be 'tst'?

gregschohn · 2023-03-20T15:32:35Z

TrafficReplayer/build.gradle

+            compileClasspath += main.output + test.output
+            runtimeClasspath += main.output + test.output


do you know what compileClasspath/runtimeClasspath are set to beforehand (asking because of the '+=")?

compileClasspath defaults to the compile configuration and runtimeClasspath defaults to the sourceSet's output + runtime configuration

Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

lewijacn · 2023-03-27T22:22:06Z

TrafficReplayer/build.gradle

+    implementation group: 'org.apache.logging.log4j', name: 'log4j-api', version: '2.20.0'
+    implementation group: 'org.apache.logging.log4j', name: 'log4j-core', version: '2.20.0'
+    testImplementation 'org.junit.jupiter:junit-jupiter:5.9.2'
+    annotationProcessor 'org.projectlombok:lombok:1.18.10'


I noticed this in my testing. Looks like we should bump this to 1.18.22: https://stackoverflow.com/questions/66801256/java-lang-illegalaccesserror-class-lombok-javac-apt-lombokprocessor-cannot-acce

I'll bump that along with a few more things that the mend security check pointed out. https://github.com/opensearch-project/opensearch-migrations/pull/118/checks?check_run_id=12176201328

…curity check Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

lewijacn · 2023-03-29T02:00:19Z

Couple things then I'm fine to approve:

I don't want to mess with the gradle configuration too much while this is still a sub directory of this repo as its hard to tell what is an actual problem or what is Intelij having trouble reconciling. But we should definitely make sure this works well when we move it out and is properly picked up by Intelij.

I was able to build successfully after fixing a spot bugs issue in line 282 of TrafficReplayer.java. Not sure if you've looked into this but if @gregschohn does not have concern would add it in
Bug: Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behaviour to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly
Original:
try (InputStreamReader r = new InputStreamReader(fis)) {
Change:
try (InputStreamReader r = new InputStreamReader(fis, StandardCharsets.UTF_8)) {

Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

lewijacn

LGTM

gregschohn

You can also delete the IntelliJ project file (TrafficReplayer.iml)
I'm sure that I will soon regret not looking at the checkstyle.xml file carefully :)

okhasawn · 2023-03-29T22:10:00Z

You can also delete the IntelliJ project file (TrafficReplayer.iml) I'm sure that I will soon regret not looking at the checkstyle.xml file carefully :)

@gregschohn I'll remove that file in one of the other PR's that I have open.
The checkstyle.xml file is just the default google java style guideline.

Migrations-1015 - DRAFT - Add builscript for replayer

340b4c8

Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

okhasawn added 3 commits March 10, 2023 12:10

Add spotbugs support

4ebbd26

Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

Add spotbugs to automatically run when building replayer

535558b

Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

Add license header

b37dea3

Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

okhasawn marked this pull request as ready for review March 13, 2023 16:47

okhasawn requested a review from a team as a code owner March 13, 2023 16:47

okhasawn requested a review from gregschohn March 13, 2023 16:47

okhasawn changed the title ~~Migrations-1015 - DRAFT - Add buildscript for replayer~~ Migrations-1015 - Add buildscript for replayer Mar 13, 2023

Proposed changes by PR opensearch-project#119 will break this, this c…

70b10b9

…ommit fixes that Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

gregschohn reviewed Mar 20, 2023

View reviewed changes

Remove what is no longer necessary due to using gradle build

3dfcc83

Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

lewijacn reviewed Mar 27, 2023

View reviewed changes

okhasawn added 2 commits March 28, 2023 13:58

Updating versions of checkstyle + spotbugs + lombok - to pass mend se…

a69535e

…curity check Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

Merge remote-tracking branch 'origin/main' into replayerBuild

ff51474

specifying a charset for inputStreamReader to fix a spotbugs issue

c4a4cf1

Signed-off-by: Omar Khasawneh <okhasawn@amazon.com>

lewijacn approved these changes Mar 29, 2023

View reviewed changes

gregschohn approved these changes Mar 29, 2023

View reviewed changes

okhasawn merged commit b6fbcc2 into opensearch-project:main Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrations-1015 - Add buildscript for replayer #118

Migrations-1015 - Add buildscript for replayer #118

okhasawn commented Mar 9, 2023 •

edited

Loading

codecov-commenter commented Mar 9, 2023 •

edited

Loading

gregschohn commented Mar 9, 2023

okhasawn commented Mar 9, 2023

gregschohn left a comment

gregschohn Mar 20, 2023

gregschohn Mar 20, 2023

okhasawn Mar 21, 2023

gregschohn Mar 20, 2023

gregschohn Mar 20, 2023

okhasawn Mar 20, 2023

lewijacn Mar 27, 2023

okhasawn Mar 27, 2023

lewijacn commented Mar 29, 2023

lewijacn left a comment

gregschohn left a comment

okhasawn commented Mar 29, 2023 •

edited

Loading

		compileClasspath += main.output + test.output
		runtimeClasspath += main.output + test.output

Migrations-1015 - Add buildscript for replayer #118

Migrations-1015 - Add buildscript for replayer #118

Conversation

okhasawn commented Mar 9, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Mar 9, 2023 • edited Loading

Codecov Report

gregschohn commented Mar 9, 2023

okhasawn commented Mar 9, 2023

gregschohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewijacn commented Mar 29, 2023

lewijacn left a comment

Choose a reason for hiding this comment

gregschohn left a comment

Choose a reason for hiding this comment

okhasawn commented Mar 29, 2023 • edited Loading

okhasawn commented Mar 9, 2023 •

edited

Loading

codecov-commenter commented Mar 9, 2023 •

edited

Loading

okhasawn commented Mar 29, 2023 •

edited

Loading