Conversation
Codecov Report
@@ Coverage Diff @@
## master #82 +/- ##
=========================================
Coverage 53.52% 53.52%
Complexity 275 275
=========================================
Files 39 39
Lines 1659 1659
Branches 206 206
=========================================
Hits 888 888
Misses 689 689
Partials 82 82 Continue to review full report at Codecov.
|
|
LGMT. Thanks |
gaojieliu
left a comment
gaojieliu
left a comment
There was a problem hiding this comment.
Left some minor comments, and look good overall.
| } | ||
| this.generatedSerDesNum.incrementAndGet(); | ||
| } catch (Exception e) { | ||
| LOGGER.error("Fast serdes class generation failed"); |
There was a problem hiding this comment.
Shall we print out the full error stacktrace here?
There was a problem hiding this comment.
The exceptions are handled in other functions. So a stack trace here may not provide any useful information.
| } else if (this.generatedSerDesNum.get() > this.generatedFastSerDesLimit) { | ||
| LOGGER.debug("Generated serdes number {}, with fast serdes limit set to {}", this.generatedSerDesNum.get(), this.generatedFastSerDesLimit); | ||
| } | ||
| this.generatedSerDesNum.incrementAndGet(); |
There was a problem hiding this comment.
Race condition could happen here since the check and update are happening in different places.
I don't think it is a big concern considering the default thread pool size is 2, so in the worst scenario, the total number of the generated classes could be one more than the specified limit.
But it is good to call it out here, and we need to use AtomicInteger here since there is no difference from an Integer according to the actual usage.
wxn225
force-pushed
the
feature/addLimitForCache
branch
from
8464663 to
36419bd
Compare
4 participants
No description provided.