Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Persistance concurrency bug #323

Merged
merged 6 commits into from
Aug 3, 2020
Merged

Persistance concurrency bug #323

merged 6 commits into from
Aug 3, 2020

Conversation

yojs
Copy link
Contributor

@yojs yojs commented Jul 30, 2020

Issue #: #324

Description of changes:
This change makes us print less stacktrace on errors that are expected. Also resolves a concurrency bug while threads doing writes and while the file is being rotated.

Tests:
Added a test

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@yojs yojs requested review from ktkrg and rguo-aws July 30, 2020 19:29
@yojs yojs added the bug Something isn't working label Jul 30, 2020
@yojs yojs force-pushed the persistance-concurrency-bug branch from 730ca92 to 5a1a4a5 Compare July 30, 2020 21:00
@yojs yojs requested a review from sidheart July 30, 2020 21:01
@codecov
Copy link

codecov bot commented Jul 30, 2020

Codecov Report

Merging #323 into master will decrease coverage by 0.27%.
The diff coverage is 44.59%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #323      +/-   ##
============================================
- Coverage     67.15%   66.88%   -0.28%     
+ Complexity     2034     2033       -1     
============================================
  Files           297      297              
  Lines         13063    13095      +32     
  Branches       1081     1086       +5     
============================================
- Hits           8773     8759      -14     
- Misses         3910     3954      +44     
- Partials        380      382       +2     
Impacted Files Coverage Δ Complexity Δ
...erformanceanalyzer/rca/persistence/FileRotate.java 52.94% <16.66%> (-17.06%) 10.00 <1.00> (+1.00) ⬇️
...manceanalyzer/rca/persistence/SQLitePersistor.java 72.98% <52.38%> (-7.77%) 23.00 <0.00> (ø)
...ormanceanalyzer/rca/persistence/PersistorBase.java 82.60% <85.71%> (-6.87%) 20.00 <0.00> (ø)
...ch/performanceanalyzer/rca/persistence/FileGC.java 92.72% <100.00%> (ø) 14.00 <0.00> (ø)
...rch/performanceanalyzer/rca/framework/api/Rca.java 80.00% <0.00%> (-14.29%) 5.00% <0.00%> (ø%)
.../performanceanalyzer/rca/framework/core/Stats.java 97.82% <0.00%> (-2.18%) 22.00% <0.00%> (-1.00%)
...csearch/performanceanalyzer/rca/RcaController.java 80.76% <0.00%> (-0.55%) 38.00% <0.00%> (-1.00%)

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 1ac07f1...ed3410c. Read the comment docs.

@yojs yojs force-pushed the persistance-concurrency-bug branch from 5a1a4a5 to 02726fa Compare July 30, 2020 21:42
try {
LOG.debug("Trying to create a summary table: {} that references {}", tableName, referenceTableName);
Table referenceTable = DSL.table(referenceTableName);
CreateTableConstraintStep constraintStep = create.createTable(tableName)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume creating the SQL statement itself does not throw any exception. Can we leave this part out of the try block ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The DSLContext can be uninitialized if we are not using synchronization between the threads. I saw a case where the table was added to hastable but the columns for the table were missing. We gather some date to create the createStatement and it the gathering part we can run into exceptions.
These cases are fixed now but it would be good to catch and log exception. But you are right, the scope of exceptions is larger and so I have added code to catch exception and not just DataAcessException.

@yojs yojs force-pushed the persistance-concurrency-bug branch from 65a9c4d to ed3410c Compare August 3, 2020 16:46
@ktkrg ktkrg self-requested a review August 3, 2020 17:42
@ktkrg ktkrg merged commit b5f58fb into master Aug 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants