Conversation
rishav-karanjit
left a comment
rishav-karanjit
left a comment
There was a problem hiding this comment.
Overall looks good. All my comments are just nits except two comments/questions in DBESDKBenchmark.java.
| diff-generated-code: false | ||
| update-and-regenerate-mpl: true | ||
|
|
||
| - name: Setup Java 8 |
There was a problem hiding this comment.
With step (Setup Java ${{ matrix.java-version }}) already in the yml file, this step seems redundant.
There was a problem hiding this comment.
Question: In Java, do we expect to push the jar file?
| // Wait for all workers to complete | ||
| CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).get(); | ||
| } finally { | ||
| executor.shutdown(); |
There was a problem hiding this comment.
executor.shutdown() tells the pool to stop accepting new tasks and start shutting down. It returns immediately without waiting for threads to actually terminate. Should we wait for all the threads to be terminated?
When next concurrent test starts in the same run, I can see the next test being affected by the previous thread if threads are still running.
There was a problem hiding this comment.
Line 661 does a get on all which is blocking. And the shutdown happens after all the concurrent threads are done so no new concurrent run happens in the same executor (the service is created at line 623 which is local to the method.
| } | ||
| Thread.sleep(SAMPLING_INTERVAL_MS); | ||
| } | ||
| } catch (InterruptedException e) { |
There was a problem hiding this comment.
This try catch only catches one Exception. If other exception (i.e OutOfMemoryError, NullPointerException, etc) occurs the thread seem to die silently which produces incorrect benchmark.
There was a problem hiding this comment.
My understanding is in Java, program crashes only when exception occur in main thread.
| *.iml | ||
|
|
||
| # Mac OS X | ||
| .DS_Store |
There was a problem hiding this comment.
| .DS_Store | |
| .DS_Store | |
| # Package Files | |
| *.jar |
| working-directory: ./${{matrix.benchmark-dir}}/benchmarks/java | ||
| run: | | ||
| ./gradlew run --args="--config ../config/test-scenarios.yaml --quick" | ||
| ./gradlew run --args="--config ../config/test-scenarios.yaml --quick --legacy-override" |
There was a problem hiding this comment.
Q: What's the --legacy-override?
There was a problem hiding this comment.
I added it so that I could get DDBEC metrics
| private double totalMemoryGb; | ||
|
|
||
| // Constructors | ||
| public BenchmarkResult() {} |
There was a problem hiding this comment.
Why do we even need this??
| this.testName = testName; | ||
| } | ||
|
|
||
| public String getLanguage() { |
There was a problem hiding this comment.
I know this is based on ESDK but maybe we can use Lombok or data classes in newer versions of Java?
| // Wait for all workers to complete | ||
| CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).get(); | ||
| } finally { | ||
| executor.shutdown(); |
There was a problem hiding this comment.
Line 661 does a get on all which is blocking. And the shutdown happens after all the concurrent threads are done so no new concurrent run happens in the same executor (the service is created at line 623 which is local to the method.
3 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.