Minor fixes in IOException catch and metrics registry #1170
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. When log segment instatiation encounters IOException, no matter freeing log segment succeeds or not, ensure that store exception is correclty thrown in ensureCapacity method. 2. Register the custom percentile metrics during RouterMetrics initialization rather than register them every time adaptive tracker is created. This will avoid unnecessary overhead.
jsjtzyy
commented
May 9, 2019
| @@ -69,7 +69,7 @@ public CryptoJobHandlerTest() throws IOException, GeneralSecurityException { | |||
| cryptoService = new GCMCryptoServiceFactory(verifiableProperties, REGISTRY).getCryptoService(); | |||
| cryptoJobHandler = new CryptoJobHandler(DEFAULT_THREAD_COUNT); | |||
| referenceClusterMap = new MockClusterMap(); | |||
| routerMetrics = new NonBlockingRouterMetrics(referenceClusterMap); | |||
| routerMetrics = new NonBlockingRouterMetrics(referenceClusterMap, null); | |||
There was a problem hiding this comment.
I purposely set RouterConfig = null when creating NonBlockingRouterMetrics in several tests. This is to verify that even if router config is mistakenly set to null in the ctor of NonBlockingRouterMetrics, there would be no failure during instantiation.
cgtz
reviewed
May 9, 2019
| @@ -470,17 +466,15 @@ private void ensureCapacity(long writeSize) throws StoreException { | |||
| new LogSegment(segmentNameAndFilename.getFirst(), newSegmentFile, segmentCapacity, metrics, true); | |||
| segmentsByName.put(segmentNameAndFilename.getFirst(), newSegment); | |||
| } catch (StoreException e) { | |||
| logger.error("Failed to create new log segment {} with store exception: ", segmentNameAndFilename.getFirst(), e); | |||
| try { | |||
There was a problem hiding this comment.
a test for this block will be good, but its ok if its too cumbersome.
Codecov Report
@@ Coverage Diff @@
## master #1170 +/- ##
============================================
- Coverage 69.96% 69.92% -0.05%
+ Complexity 5371 5358 -13
============================================
Files 427 427
Lines 32775 32770 -5
Branches 4152 4133 -19
============================================
- Hits 22931 22914 -17
- Misses 8706 8718 +12
Partials 1138 1138
Continue to review full report at Codecov.
|
cgtz
reviewed
May 9, 2019
| @@ -13,6 +13,9 @@ | |||
| */ | |||
| package com.github.ambry.store; | |||
|
|
|||
| import java.util.Objects; | |||
|
|
|||
|
|
|||
| public class StoreException extends Exception { | |||
| public static final String INTERNAL_ERROR_STR = | |||
There was a problem hiding this comment.
Can these be private or package-private
There was a problem hiding this comment.
changed it to package-private
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
freeing log segment succeeds or not, ensure that store exception is
correctly thrown in ensureCapacity method.
initialization rather than register them every time adaptive tracker is
created. This will avoid unnecessary overhead.