-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Github-issue#1048 : s3 object index. #2586
Github-issue#1048 : s3 object index. #2586
Conversation
Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>
/** | ||
* Class responsible for creation of s3 key pattern based on date time stamp | ||
*/ | ||
public class S3ObjectIndex { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S3ObjectIndexUtility
may be a more accurate name for this class, since it is performing validations and creation of the s3 key pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>
public static String getObjectNameWithDateTimeId(final String indexAlias) { | ||
DateTimeFormatter dateFormatter = getDatePatternFormatter(indexAlias); | ||
String suffix = (dateFormatter != null) ? dateFormatter.format(getCurrentUtcTime()) : ""; | ||
return indexAlias.replaceAll(TIME_PATTERN_REGULAR_EXPRESSION, "") + suffix + "-" + getTimeNanos() + "-" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can we simplify it to
indexAlias.replaceAll(TIME_PATTERN_REGULAR_EXPRESSION, suffix + "-" + getTimeNanos() + "-" + UUID.randomUUID())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
/** | ||
* Validate the index with the regular expression pattern. Throws exception if validation fails | ||
*/ | ||
public static DateTimeFormatter getDatePatternFormatter(final String indexAlias) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a more precise name for the method, e.g. validateAndGetDateTimeFormatter
or validateAndCreateDateTimeFormatter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the method name to validateAndGetDateTimeFormatter
.
} | ||
|
||
@Test | ||
void testgetObjectPathPrefix_not_equal() throws IllegalArgumentException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is not_equal
correct in the test case name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified the method name based on the functionality.
@Test | ||
void testObjectTimePatterns_equal() throws IllegalArgumentException { | ||
|
||
DateTimeFormatter expectedIndex = S3ObjectIndexUtility.getDatePatternFormatter("events-%{yyyy-MM-dd}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expectedIndex -> expectedDateTimeFormatter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
void testObjectTimePatterns_equal() throws IllegalArgumentException { | ||
|
||
DateTimeFormatter expectedIndex = S3ObjectIndexUtility.getDatePatternFormatter("events-%{yyyy-MM-dd}"); | ||
DateTimeFormatter actualIndex = S3ObjectIndexUtility.getDatePatternFormatter("events-%{yyyy-MM-dd}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actualIndex -> actualDateTimeFormatter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it is recommended to use a slightly different indexAlias as long as the same datetime pattern is contained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
@Test | ||
void test_utc_current_time() throws IllegalArgumentException { | ||
|
||
ZonedDateTime expectedIndex = S3ObjectIndexUtility.getCurrentUtcTime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar as above. Please change the name of the variable to be accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
@Test | ||
void testObjectAliasWithDatePrefix_equal() throws IllegalArgumentException { | ||
|
||
String expectedIndex = S3ObjectIndexUtility.getObjectNameWithDateTimeId("events-%{yyyy-MM-dd}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #2586 +/- ##
============================================
+ Coverage 93.40% 93.46% +0.06%
- Complexity 2168 2186 +18
============================================
Files 257 258 +1
Lines 6064 6107 +43
Branches 490 497 +7
============================================
+ Hits 5664 5708 +44
+ Misses 270 268 -2
- Partials 130 131 +1
|
Description
Issues Resolved
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.