Skip to content

Conversation

@kianfar77
Copy link
Collaborator

What changes are proposed in this pull request?

Adds minor changes to work with logging
Put bigbegn write options in common.BgenOptions

@codecov
Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #65 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #65   +/-   ##
=======================================
  Coverage   94.11%   94.11%           
=======================================
  Files          74       74           
  Lines        3569     3569           
  Branches      331      332    +1     
=======================================
  Hits         3359     3359           
  Misses        210      210
Impacted Files Coverage Δ
...o/projectglow/common/logging/HlsUsageLogging.scala 100% <ø> (ø) ⬆️
.../scala/io/projectglow/bgen/BigBgenDatasource.scala 100% <ø> (ø) ⬆️
...rojectglow/transformers/pipe/PipeTransformer.scala 96.49% <100%> (+0.06%) ⬆️
...cala/io/projectglow/common/datasourceOptions.scala 100% <100%> (ø) ⬆️

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 4477e21...27b227b. Read the comment docs.

@kianfar77 kianfar77 requested a review from karenfeng November 8, 2019 19:11
Copy link
Collaborator

@karenfeng karenfeng left a comment

Choose a reason for hiding this comment

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

Looks good overall, just one question.

private val ENV_PREFIX = "env_"
private val INPUT_FORMATTER_PREFIX = "in_"
private val OUTPUT_FORMATTER_PREFIX = "out_"
val LOGGING_BLOB_KEY = "pipeCmdTool"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we moving this here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For all other features, like vcf/bgen/plink read/write and variant normalizer transformer, the options which are defined inside the respective packages are used as keys for the maps in the logging json blobs. I do not want this one to be an exception and be defined outside of pipe transformer. This makes the logging code cleaner on the universe side as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. Can we move the transformer options to a higher-level object as we did for the datasource options for transparency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure I can. But if ok with you let's do it in another PR because I want to get the universe PR review moving froward with data and observability teams and that depends on this PR. Moving options to higher level can be done independently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, that works.

@kianfar77 kianfar77 requested a review from karenfeng November 8, 2019 19:21
@kianfar77 kianfar77 merged commit 06a8653 into projectglow:master Nov 8, 2019
henrydavidge added a commit to henrydavidge/glow that referenced this pull request Jun 22, 2020
* Remove SpecificInternalRow buffer in RowConverter

* comment

Signed-off-by: Henry Davidge <[email protected]>
henrydavidge pushed a commit to henrydavidge/glow that referenced this pull request Jun 22, 2020
* minor changes needed for logging

Signed-off-by: kianfar77 <[email protected]>

* logging plink blob and minor changes

Signed-off-by: kianfar77 <[email protected]>

* bug fix

Signed-off-by: kianfar77 <[email protected]>

* moving pipe blob key

Signed-off-by: kianfar77 <[email protected]>

* drop HlsBlobKeys

Signed-off-by: kianfar77 <[email protected]>

* changes for logging

Signed-off-by: kianfar77 <[email protected]>

Signed-off-by: Henry Davidge <[email protected]>
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.

2 participants