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

Use StringUtils instead of NumberUtils to check timestamp string #360

Merged
merged 2 commits into from
Aug 8, 2020

Conversation

ktkrg
Copy link
Contributor

@ktkrg ktkrg commented Aug 8, 2020

Issue #:
Fixes #359

Description of changes:
This change updates a check where we used NumberUtils.isCreatable(timestamp); to StringUtils.isNumeric(timestamp);

The check we want to do is that the value we received from the writer is actually an epoch which is checked more strongly with the .isNumeric() check than .isCreatable() as the latter allows other forms of numbers(signed, hexadecimal, scientific notation, decimals) as valid numbers.

Removes some unused imports as well.
Tests:
build passes.

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

@ktkrg ktkrg requested review from sruti1312 and khushbr August 8, 2020 02:19
@ktkrg ktkrg self-assigned this Aug 8, 2020
@ktkrg ktkrg added the bug Something isn't working label Aug 8, 2020
@codecov
Copy link

codecov bot commented Aug 8, 2020

Codecov Report

Merging #360 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #360      +/-   ##
============================================
- Coverage     67.21%   67.17%   -0.04%     
+ Complexity     2088     2086       -2     
============================================
  Files           301      301              
  Lines         13326    13326              
  Branches       1104     1104              
============================================
- Hits           8957     8952       -5     
- Misses         3966     3970       +4     
- Partials        403      404       +1     
Impacted Files Coverage Δ Complexity Δ
...lyzer/config/overrides/ConfigOverridesApplier.java 72.72% <100.00%> (ø) 13.00 <0.00> (ø)
...o/elasticsearch/performanceanalyzer/core/Util.java 62.50% <0.00%> (-8.34%) 5.00% <0.00%> (ø%)
.../performanceanalyzer/rca/framework/api/Metric.java 76.19% <0.00%> (-4.77%) 8.00% <0.00%> (-1.00%)
...ceanalyzer/rca/store/rca/hotshard/HotShardRca.java 85.55% <0.00%> (-1.12%) 16.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 79841f2...238f45e. Read the comment docs.

@sruti1312 sruti1312 merged commit e03a120 into master Aug 8, 2020
@ktkrg ktkrg deleted the ktkrg-numberutils branch August 8, 2020 02:31
khushbr pushed a commit that referenced this pull request Sep 30, 2020
Changes for 1 package (OpenDistroPerformanceAnalyzerEngine), pushed in snapshot...

  https://code.amazon.com/snapshots/partsrut/2020-08-08T02-49-04

Changes for OpenDistroPerformanceAnalyzerEngine package:

0194f9f Fix failing reaction wheel tests
bfdb93d Merge remote-tracking branch 'upstream/master'
e03a120 Use StringUtils instead of NumberUtils to check timestamp string (#360)
79841f2 Expose queue rejection time period setting in rca.conf (#356)
f9dfda5 Implement isMuted method in DummyAction (#355)
7ede0b9 Reader changes for dynamic enable/disable of RCA graph components (#325)
ee58207 Fix bug in publisher to support cool off period on a per node basis (#351)

cr https://code.amazon.com/reviews/CR-31344155
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.

NumberUtils.isCreatable() is too wide of a check
3 participants