Allow putDimensionSet after setDimensions #62
Conversation
Before this change, if you call MetricDirective.setDimensions and then MetricDirective.putDimensionSet, you get UnsupportedOperationException. With this change, everything works as expected.
There was a problem hiding this comment.
Thanks for the PR! One blocking comment, but would like @yaozhaoy to look as well.
| @@ -22,6 +22,8 @@ | |||
| import com.fasterxml.jackson.databind.MapperFeature; | |||
| import com.fasterxml.jackson.databind.ObjectMapper; | |||
| import java.util.Arrays; | |||
| import java.util.Collections; | |||
|
|
|||
There was a problem hiding this comment.
Build is failing due to the added whitespace. Do you mind removing this?
Starting a Gradle Daemon (subsequent builds will be faster)
--
115 | > Task :compileJava
116 | > Task :processResources NO-SOURCE
117 | > Task :classes
118 | > Task :jar
119 | > Task :javadoc
120 | > Task :javadocJar
121 | > Task :sourcesJar
122 | > Task :assemble
123 | > Task :spotlessJava
124 | > Task :spotlessJavaCheck FAILED
125 |
126 | FAILURE: Build failed with an exception.
127 |
128 | * What went wrong:
129 | Execution failed for task ':spotlessJavaCheck'.
130 | > The following files had format violations:
131 | src/test/java/software/amazon/cloudwatchlogs/emf/model/MetricDirectiveTest.java
132 | @@ -23,7 +23,6 @@
133 | import?com.fasterxml.jackson.databind.ObjectMapper;
134 | import?java.util.Arrays;
135 | import?java.util.Collections;
136 | -
137 | import?org.junit.Test;
138 |
139 | public?class?MetricDirectiveTest?{
140 | Run './gradlew :spotlessApply' to fix these violations.
141 |
142 | * Try:
143 | Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.
There was a problem hiding this comment.
Whoops. Fixed that. Thanks.
| @@ -86,7 +86,7 @@ void putMetric(String key, double value, Unit unit) { | |||
| */ | |||
| void setDimensions(List<DimensionSet> dimensionSets) { | |||
| shouldUseDefaultDimension = false; | |||
| dimensions = dimensionSets; | |||
| dimensions = new ArrayList<>(dimensionSets); | |||
There was a problem hiding this comment.
This will cause an array copy and an unnecessary allocation. If ArrayList is required here, then we should probably accept it in the method signature rather than allocate a new one. However, that is a breaking change. So, I'm fine with this fix for now.
There was a problem hiding this comment.
I think we should fix the issue at the MetricsContext.java class, as the issue was introduced by the usage of Arrays.asList. We can change to use Lists.newArrayList in the Guava library. We need add guava to our dependencies to use this library.
There was a problem hiding this comment.
This will cause an array copy and an unnecessary allocation. If ArrayList is required here, then we should probably accept it in the method signature rather than allocate a new one. However, that is a breaking change. So, I'm fine with this fix for now.
ArrayList isn't required. List is required -- at least a List implementation that has all of its methods implemented. Plus, would you want to directly accept a List and then hold a reference to it? Then the person who gave you the list can modify it while you're holding on to it.
I think we should fix the issue at the MetricsContext.java class, as the issue was introduced by the usage of Arrays.asList. We can change to use Lists.newArrayList in the Guava library. We need add guava to our dependencies to use this library.
I considered making the change to MetricsContext. To me it's more brittle. Changing the setDimension implementation fixes the problem once and for all. Changing 1 call site fixes it just for that case. For example I have a test that does metricDirective.setDimensions(Collections.singletonList(DimensionSet.of("Version", "1"))); that would still break if I just change the call site.
There was a problem hiding this comment.
Thanks for the prompt feedback. Let me know what y'all think and I'll send a new revision.
There was a problem hiding this comment.
That sounds reasonable to me. :+1 Please go ahead to submit a new revision.
There was a problem hiding this comment.
Thanks. I had to update the commit, and then make a new remote branch since the hash has changed. I don't know enough about pull requests to know how to reference the new branch in this pull request. So I made a new one #63
jaredcnance
requested review from
jaredcnance
and removed request for
jaredcnance
Before this change, if you call MetricDirective.setDimensions and
then MetricDirective.putDimensionSet, you get UnsupportedOperationException.
With this change, everything works as expected.
Issue #, if available:
59
Description of changes:
Commit message has a description.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.