Skip to content
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

Add a reset() method to ProgressLoggerInterface. #1184

Merged
merged 2 commits into from
Oct 5, 2018

Conversation

nh13
Copy link
Member

@nh13 nh13 commented Oct 3, 2018

I have two uses for this method:

  1. I may create a ProgressLogger earlier, but want to set the startTime so it is right before the logger is being used. Yes, I can create the ProgressLogger at the last minute.
  2. I may want to reuse a progress logger.

@codecov-io
Copy link

codecov-io commented Oct 3, 2018

Codecov Report

Merging #1184 into master will increase coverage by 0.009%.
The diff coverage is 87.5%.

@@              Coverage Diff               @@
##             master     #1184       +/-   ##
==============================================
+ Coverage      68.4%   68.409%   +0.009%     
- Complexity     8014      8016        +2     
==============================================
  Files           541       542        +1     
  Lines         32690     32693        +3     
  Branches       5529      5529               
==============================================
+ Hits          22360     22365        +5     
+ Misses         8126      8125        -1     
+ Partials       2204      2203        -1
Impacted Files Coverage Δ Complexity Δ
.../htsjdk/samtools/util/ProgressLoggerInterface.java 0% <0%> (ø) 0 <0> (?)
...a/htsjdk/samtools/util/AbstractProgressLogger.java 42.593% <100%> (+2.208%) 7 <1> (+1) ⬆️
...samtools/util/AsyncBlockCompressedInputStream.java 76% <0%> (+4%) 13% <0%> (+1%) ⬆️

Copy link
Member

@tfenne tfenne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's give @jacarey and @lbergelson some time to weigh in, but if they don't object I'll merge this in a day or two.

@lbergelson
Copy link
Member

This seems fine, it is a breaking API change which is gross, but I suspect the only downstream implementer of ProgressLogger is the GATK so it's probably not a huge deal in practice.

It feel like maybe this is a problem better solved by just creating it when it's needed, but maybe there's some good use for it that I'm not seeing.

@tfenne
Copy link
Member

tfenne commented Oct 3, 2018

If you're worried about the breaking API change we could ask @nh13 to add a default no-op implementation of reset() in the interface.
I'll admit I would find this useful too on occasion. I have tools where I create 2-3 different loggers, one for each phase of work, and it would be be convenient to reuse a single one.

@pshapiro4broad
Copy link
Contributor

pshapiro4broad commented Oct 4, 2018

You can prevent this being a breaking change by making reset() a default method in the interface. This is pretty much the use case default methods were designed for. Sorry, missed that @tfenne already covered this. Yes, please add a default method.

@@ -124,6 +124,16 @@ public boolean record(final SAMRecord... recs) {
/** Returns the number of seconds since progress tracking began. */
public long getElapsedSeconds() { return (System.currentTimeMillis() - this.startTime) / 1000; }

/** Resets the start time to now and the number of records to zero. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, you want the javadoc on the baseclass or interface, so it's inherited by all subclassers and so a user can see the javadoc when dealing with the interface type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but I am going to pass on this change to be consistent with how ProgressLoggerInterface currently is written.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @pshapiro4broad but lets handle that separately.

@nh13
Copy link
Member Author

nh13 commented Oct 5, 2018

I certainly can create the object right before it's used most of the time (I could use lazy in scala), but sometimes, there may be an initial delay from creation time to when the first record (and subsequent records) start processing that I don't want to affect the start time.

The alternative I was considering was having a way to configure ProgressLogger to set the startTime upon recording the first record.

@lbergelson
Copy link
Member

@nh13 This seems like a fine solution, setting it when you record the first record is usually alright, but it has a similar issue to setting it at construction time if for some reason the first record is unusually slow or if you only have a few long operations being recorded. 👍 to this change with the default method.

@lbergelson lbergelson merged commit a7214ca into master Oct 5, 2018
@lbergelson lbergelson deleted the nh_progress_logger_reset branch October 5, 2018 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants