make ArchiveRecord a trait#186
Conversation
|
Thanks @helgeho - I should be able to kick the tires on this later this week (or early next in worst case - it's a tough time of term). |
Codecov Report
@@ Coverage Diff @@
## master #186 +/- ##
=========================================
+ Coverage 67.24% 67.34% +0.1%
=========================================
Files 32 32
Lines 635 637 +2
Branches 124 124
=========================================
+ Hits 427 429 +2
Misses 167 167
Partials 41 41
Continue to review full report at Codecov.
|
| val getDomain: String = ExtractDomain(getUrl) | ||
|
|
||
| val getImageBytes: Array[Byte] = { | ||
| if (getContentString.startsWith("HTTP/")) |
There was a problem hiding this comment.
for consistency, add braces also?
There was a problem hiding this comment.
I didn't change anything there, that's exactly your current https://github.com/archivesunleashed/aut/blob/master/src/main/scala/io/archivesunleashed/spark/archive/io/ArchiveRecord.scala , I only wanted to add the trait, not make your code more / less consistent, because I do not know how your policy is in terms of coding styles (personally though, I also prefer no braces for one-liners)
|
https://github.com/helgeho/ArchiveSpark-AUT-bridge is also updated now, please check it out, give it a try! |
|
Neat! Although you'll want to change your documentation to reflect the new package structure. |
|
oh, that's right, thanks for the heads up, I'll do that soon |
4 participants
GitHub issue(s):
What does this Pull Request do?
This commit introduces a trait that describes the interface of an
ArchiveRecordin AUT. This allows for alternative implementations of this core record type in AUT, such as the one provided by ArchiveSpark-AUT-bridge. The original AUT implementation ofArchiveRecord, which is instantiated by AUT'sRecordLoader, is moved to theArchiveRecordImplclass.This was already done before by #175 and reverted by #181 because the change of moving
ISO8601into a static object caused some issues with certain collections (see below). Therefore, this time,ISO8601remains part ofArchiveRecordImpl.How should this be tested?
As this is a pure refactoring without changing any logic, a simple compilation + smoke test should be sufficient. However, as it turns out, the little additional change of moving
ISO8601into a static object in the first attempt of this change (#175) caused strange behaviors with some collections (probably when running on multiple cores per executor, sinceSimpleDateFormatis not thread-safe as pointed out by @anjackson, see #181). Hence, it should be better tested with different kinds of collections to make sure there are no unexpected side effects involved.Additional Notes:
The first attempt to make ArchiveRecord a trait (#175) was reverted by #181. This time nothing else is changed except for the trait (
ISO8601remains part ofArchiveRecordImpl). @ruebot and @ianmilligan1 please test it as you did last time (#181), I hope it is not causing problems this time.