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

Save metrics on close + close only once + check acc registration #296

Merged
merged 12 commits into from Oct 18, 2016

Conversation

ravwojdyla
Copy link
Contributor

No description provided.

@ravwojdyla ravwojdyla changed the title Metrics Save metrics on close Oct 17, 2016
@andrewsmartin
Copy link
Contributor

👍 LGTM

val scioResult = new ScioResult(result, finalState, _accumulators.values.toSeq, pipeline)
val metricsLocation = optionsAs[ScioOptions].getMetricsLocation
if (metricsLocation != null && finalState.isCompleted) {
scioResult.saveMetrics(metricsLocation)
Copy link
Contributor

Choose a reason for hiding this comment

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

This only works with blocking runner? Should support non-blocking ones by setting a Future handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. initially thought that supporting non blocking one would be an overkill. but it would be most consistent. need to move things around a bit.

@ravwojdyla ravwojdyla changed the title Save metrics on close Save metrics on close + close only once + check acc registration Oct 18, 2016
@codecov-io
Copy link

codecov-io commented Oct 18, 2016

Current coverage is 82.20% (diff: 87.50%)

Merging #296 into master will increase coverage by 0.09%

@@             master       #296   diff @@
==========================================
  Files            63         63          
  Lines          2320       2332    +12   
  Methods        2058       2058          
  Messages          0          0          
  Branches        262        274    +12   
==========================================
+ Hits           1905       1917    +12   
  Misses          415        415          
  Partials          0          0          

Powered by Codecov. Last update 54d66a4...f98ffba

@nevillelyh
Copy link
Contributor

👍

@ravwojdyla ravwojdyla merged commit c19c0f7 into master Oct 18, 2016
@ravwojdyla ravwojdyla deleted the metrics branch October 18, 2016 18:46
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.

None yet

4 participants