You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/03/13 02:15:39 UTC

[GitHub] [incubator-gobblin] ZihanLi58 opened a new pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

ZihanLi58 opened a new pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925
 
 
   Dear Gobblin maintainers,
   
   Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
   
   
   ### JIRA
   - [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
       - https://issues.apache.org/jira/browse/GOBBLIN-1080
   
   
   ### Description
   - [ ] Here are some details about my PR, including screenshots (if applicable):
   Add configuration to add schema creation time in converter
   
   ### Tests
   - [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   unit test 
   
   ### Commits
   - [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
       1. Subject is separated from body by a blank line
       2. Subject is limited to 50 characters
       3. Subject does not end with a period
       4. Subject uses the imperative mood ("add", not "adding")
       5. Body wraps at 72 characters
       6. Body explains "what" and "why", not "how"
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] ZihanLi58 commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#discussion_r393322197
 
 

 ##########
 File path: gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java
 ##########
 @@ -833,6 +835,7 @@
   public static final String KAFKA_SCHEMA_REGISTRY_RETRY_TIMES = "kafka.schema.registry.retry.times";
   public static final String KAFKA_SCHEMA_REGISTRY_RETRY_INTERVAL_IN_MILLIS =
       "kafka.schema.registry.retry.interval.inMillis";
+  public static final String SCHEMA_CREATION_TIME_KEY = "CreatedOn";
 
 Review comment:
   Will address

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#discussion_r392464622
 
 

 ##########
 File path: gobblin-core-base/src/main/java/org/apache/gobblin/converter/filter/AvroProjectionConverter.java
 ##########
 @@ -74,10 +74,15 @@ public AvroProjectionConverter init(WorkUnitState workUnit) {
    */
   @Override
   public Schema convertSchema(Schema inputSchema, WorkUnitState workUnit) throws SchemaConversionException {
+    Schema outputSchema = inputSchema;
     if (this.fieldRemover.isPresent()) {
-      return this.fieldRemover.get().removeFields(inputSchema);
+      outputSchema = this.fieldRemover.get().removeFields(inputSchema);
     }
-    return inputSchema;
+    if(workUnit.getPropAsBoolean(ConfigurationKeys.CONVERTER_AVRO_INCLUDE_SCHEMA_CREATION_TIME,
+        ConfigurationKeys.DEFAULT_CONVERTER_AVRO_INCLUDE_SCHEMA_CREATION_TIME)) {
 
 Review comment:
   what if a pipeline needs this but not using this converter?  shall we consider to add it into higher level ? 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] codecov-io edited a comment on issue #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#issuecomment-598522021
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=h1) Report
   > Merging [#2925](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/5796bb4c890cf4c7e48c061907232289cfb08ab9&el=desc) will **increase** coverage by `0.00%`.
   > The diff coverage is `53.84%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##             master    #2925   +/-   ##
   =========================================
     Coverage     45.46%   45.47%           
   + Complexity     9109     9108    -1     
   =========================================
     Files          1936     1936           
     Lines         73059    73070   +11     
     Branches       8051     8054    +3     
   =========================================
   + Hits          33217    33228   +11     
   + Misses        36782    36778    -4     
   - Partials       3060     3064    +4     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...pache/gobblin/configuration/ConfigurationKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29uZmlndXJhdGlvbi9Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
   | [...c/main/java/org/apache/gobblin/util/AvroUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQXZyb1V0aWxzLmphdmE=) | `57.33% <0.00%> (-0.40%)` | `81.00 <0.00> (ø)` | |
   | [...er/GobblinTrackingEventFlattenFilterConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL2ZpbHRlci9Hb2JibGluVHJhY2tpbmdFdmVudEZsYXR0ZW5GaWx0ZXJDb252ZXJ0ZXIuamF2YQ==) | `69.64% <66.66%> (-0.17%)` | `9.00 <0.00> (ø)` | |
   | [...blin/converter/filter/AvroProjectionConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL2ZpbHRlci9BdnJvUHJvamVjdGlvbkNvbnZlcnRlci5qYXZh) | `75.00% <83.33%> (ø)` | `6.00 <2.00> (ø)` | |
   | [...lin/elasticsearch/writer/FutureCallbackHolder.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tZWxhc3RpY3NlYXJjaC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9lbGFzdGljc2VhcmNoL3dyaXRlci9GdXR1cmVDYWxsYmFja0hvbGRlci5qYXZh) | `61.42% <0.00%> (-1.43%)` | `4.00% <0.00%> (ø%)` | |
   | [...e/gobblin/runtime/locks/ZookeeperBasedJobLock.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbG9ja3MvWm9va2VlcGVyQmFzZWRKb2JMb2NrLmphdmE=) | `63.33% <0.00%> (-1.12%)` | `15.00% <0.00%> (-1.00%)` | |
   | [...main/java/org/apache/gobblin/util/HadoopUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvSGFkb29wVXRpbHMuamF2YQ==) | `30.20% <0.00%> (-0.68%)` | `24.00% <0.00%> (-1.00%)` | |
   | [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `38.26% <0.00%> (+2.60%)` | `14.00% <0.00%> (+1.00%)` | |
   | [...e/gobblin/runtime/app/ServiceBasedAppLauncher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBwL1NlcnZpY2VCYXNlZEFwcExhdW5jaGVyLmphdmE=) | `49.51% <0.00%> (+5.82%)` | `12.00% <0.00%> (ø%)` | |
   | [...lin/util/filesystem/FileSystemInstrumentation.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsZXN5c3RlbS9GaWxlU3lzdGVtSW5zdHJ1bWVudGF0aW9uLmphdmE=) | `92.85% <0.00%> (+7.14%)` | `3.00% <0.00%> (ø%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=footer). Last update [5796bb4...641c5ed](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] codecov-io edited a comment on issue #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#issuecomment-598522021
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=h1) Report
   > Merging [#2925](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/5796bb4c890cf4c7e48c061907232289cfb08ab9?src=pr&el=desc) will **decrease** coverage by `0.01%`.
   > The diff coverage is `46.66%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/graphs/tree.svg?width=650&token=4MgURJ0bGc&height=150&src=pr)](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #2925      +/-   ##
   ============================================
   - Coverage     45.46%   45.45%   -0.02%     
   - Complexity     9109     9111       +2     
   ============================================
     Files          1936     1936              
     Lines         73059    73151      +92     
     Branches       8051     8074      +23     
   ============================================
   + Hits          33217    33249      +32     
   - Misses        36782    36831      +49     
   - Partials       3060     3071      +11
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...pache/gobblin/configuration/ConfigurationKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29uZmlndXJhdGlvbi9Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...c/main/java/org/apache/gobblin/util/AvroUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQXZyb1V0aWxzLmphdmE=) | `57.07% <0%> (-0.65%)` | `81 <0> (ø)` | |
   | [...er/GobblinTrackingEventFlattenFilterConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL2ZpbHRlci9Hb2JibGluVHJhY2tpbmdFdmVudEZsYXR0ZW5GaWx0ZXJDb252ZXJ0ZXIuamF2YQ==) | `69.64% <66.66%> (-0.17%)` | `9 <0> (ø)` | |
   | [...blin/converter/filter/AvroProjectionConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL2ZpbHRlci9BdnJvUHJvamVjdGlvbkNvbnZlcnRlci5qYXZh) | `75% <83.33%> (ø)` | `6 <2> (ø)` | :arrow_down: |
   | [...obblin/service/monitoring/FlowStatusGenerator.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9GbG93U3RhdHVzR2VuZXJhdG9yLmphdmE=) | `46% <0%> (-23.7%)` | `11% <0%> (ø)` | |
   | [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXIuamF2YQ==) | `71.33% <0%> (-5.14%)` | `13% <0%> (ø)` | |
   | [...e/gobblin/runtime/locks/ZookeeperBasedJobLock.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbG9ja3MvWm9va2VlcGVyQmFzZWRKb2JMb2NrLmphdmE=) | `63.33% <0%> (-1.12%)` | `15% <0%> (-1%)` | |
   | [.../apache/gobblin/runtime/api/JobExecutionState.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0pvYkV4ZWN1dGlvblN0YXRlLmphdmE=) | `79.43% <0%> (-0.94%)` | `24% <0%> (ø)` | |
   | [...he/gobblin/compaction/source/CompactionSource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25Tb3VyY2UuamF2YQ==) | `72.16% <0%> (-0.9%)` | `14% <0%> (ø)` | |
   | [...main/java/org/apache/gobblin/util/HadoopUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvSGFkb29wVXRpbHMuamF2YQ==) | `30.2% <0%> (-0.68%)` | `24% <0%> (-1%)` | |
   | ... and [6 more](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=footer). Last update [5796bb4...a7ade0f](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] codecov-io edited a comment on issue #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#issuecomment-598522021
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=h1) Report
   > Merging [#2925](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/5796bb4c890cf4c7e48c061907232289cfb08ab9&el=desc) will **decrease** coverage by `0.01%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #2925      +/-   ##
   ============================================
   - Coverage     45.46%   45.45%   -0.02%     
   - Complexity     9109     9111       +2     
   ============================================
     Files          1936     1936              
     Lines         73059    73146      +87     
     Branches       8051     8072      +21     
   ============================================
   + Hits          33217    33247      +30     
   - Misses        36782    36828      +46     
   - Partials       3060     3071      +11     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...n/java/org/apache/gobblin/converter/Converter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL0NvbnZlcnRlci5qYXZh) | `74.35% <ø> (ø)` | `11.00 <0.00> (ø)` | |
   | [...c/main/java/org/apache/gobblin/util/AvroUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQXZyb1V0aWxzLmphdmE=) | `57.07% <0.00%> (-0.65%)` | `81.00 <0.00> (ø)` | |
   | [...blin/converter/filter/AvroProjectionConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL2ZpbHRlci9BdnJvUHJvamVjdGlvbkNvbnZlcnRlci5qYXZh) | `77.77% <100.00%> (+2.77%)` | `6.00 <2.00> (ø)` | |
   | [...er/GobblinTrackingEventFlattenFilterConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL2ZpbHRlci9Hb2JibGluVHJhY2tpbmdFdmVudEZsYXR0ZW5GaWx0ZXJDb252ZXJ0ZXIuamF2YQ==) | `70.37% <100.00%> (+0.55%)` | `9.00 <0.00> (ø)` | |
   | [...obblin/service/monitoring/FlowStatusGenerator.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9GbG93U3RhdHVzR2VuZXJhdG9yLmphdmE=) | `46.00% <0.00%> (-23.70%)` | `11.00% <0.00%> (ø%)` | |
   | [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXIuamF2YQ==) | `71.33% <0.00%> (-5.14%)` | `13.00% <0.00%> (ø%)` | |
   | [...lin/elasticsearch/writer/FutureCallbackHolder.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tZWxhc3RpY3NlYXJjaC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9lbGFzdGljc2VhcmNoL3dyaXRlci9GdXR1cmVDYWxsYmFja0hvbGRlci5qYXZh) | `61.42% <0.00%> (-1.43%)` | `4.00% <0.00%> (ø%)` | |
   | [.../org/apache/gobblin/cluster/GobblinTaskRunner.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpblRhc2tSdW5uZXIuamF2YQ==) | `63.11% <0.00%> (-1.34%)` | `26.00% <0.00%> (-1.00%)` | |
   | [.../apache/gobblin/runtime/api/JobExecutionState.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0pvYkV4ZWN1dGlvblN0YXRlLmphdmE=) | `79.43% <0.00%> (-0.94%)` | `24.00% <0.00%> (ø%)` | |
   | [...he/gobblin/compaction/source/CompactionSource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25Tb3VyY2UuamF2YQ==) | `72.16% <0.00%> (-0.90%)` | `14.00% <0.00%> (ø%)` | |
   | ... and [5 more](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=footer). Last update [5796bb4...ef3225f](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] codecov-io commented on issue #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#issuecomment-598522021
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=h1) Report
   > Merging [#2925](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/5796bb4c890cf4c7e48c061907232289cfb08ab9?src=pr&el=desc) will **increase** coverage by `<.01%`.
   > The diff coverage is `53.84%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/graphs/tree.svg?width=650&token=4MgURJ0bGc&height=150&src=pr)](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #2925      +/-   ##
   ============================================
   + Coverage     45.46%   45.47%   +<.01%     
   + Complexity     9109     9108       -1     
   ============================================
     Files          1936     1936              
     Lines         73059    73070      +11     
     Branches       8051     8054       +3     
   ============================================
   + Hits          33217    33228      +11     
   + Misses        36782    36778       -4     
   - Partials       3060     3064       +4
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...pache/gobblin/configuration/ConfigurationKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29uZmlndXJhdGlvbi9Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...c/main/java/org/apache/gobblin/util/AvroUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQXZyb1V0aWxzLmphdmE=) | `57.33% <0%> (-0.4%)` | `81 <0> (ø)` | |
   | [...er/GobblinTrackingEventFlattenFilterConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL2ZpbHRlci9Hb2JibGluVHJhY2tpbmdFdmVudEZsYXR0ZW5GaWx0ZXJDb252ZXJ0ZXIuamF2YQ==) | `69.64% <66.66%> (-0.17%)` | `9 <0> (ø)` | |
   | [...blin/converter/filter/AvroProjectionConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL2ZpbHRlci9BdnJvUHJvamVjdGlvbkNvbnZlcnRlci5qYXZh) | `75% <83.33%> (ø)` | `6 <2> (ø)` | :arrow_down: |
   | [...lin/elasticsearch/writer/FutureCallbackHolder.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tZWxhc3RpY3NlYXJjaC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9lbGFzdGljc2VhcmNoL3dyaXRlci9GdXR1cmVDYWxsYmFja0hvbGRlci5qYXZh) | `61.42% <0%> (-1.43%)` | `4% <0%> (ø)` | |
   | [...e/gobblin/runtime/locks/ZookeeperBasedJobLock.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbG9ja3MvWm9va2VlcGVyQmFzZWRKb2JMb2NrLmphdmE=) | `63.33% <0%> (-1.12%)` | `15% <0%> (-1%)` | |
   | [...main/java/org/apache/gobblin/util/HadoopUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvSGFkb29wVXRpbHMuamF2YQ==) | `30.2% <0%> (-0.68%)` | `24% <0%> (-1%)` | |
   | [...in/java/org/apache/gobblin/cluster/HelixUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvSGVsaXhVdGlscy5qYXZh) | `38.26% <0%> (+2.6%)` | `14% <0%> (+1%)` | :arrow_up: |
   | [...e/gobblin/runtime/app/ServiceBasedAppLauncher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBwL1NlcnZpY2VCYXNlZEFwcExhdW5jaGVyLmphdmE=) | `49.51% <0%> (+5.82%)` | `12% <0%> (ø)` | :arrow_down: |
   | [...lin/util/filesystem/FileSystemInstrumentation.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsZXN5c3RlbS9GaWxlU3lzdGVtSW5zdHJ1bWVudGF0aW9uLmphdmE=) | `92.85% <0%> (+7.14%)` | `3% <0%> (ø)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=footer). Last update [5796bb4...641c5ed](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] ZihanLi58 commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#discussion_r393322172
 
 

 ##########
 File path: gobblin-core-base/src/main/java/org/apache/gobblin/converter/filter/AvroProjectionConverter.java
 ##########
 @@ -74,10 +74,15 @@ public AvroProjectionConverter init(WorkUnitState workUnit) {
    */
   @Override
   public Schema convertSchema(Schema inputSchema, WorkUnitState workUnit) throws SchemaConversionException {
+    Schema outputSchema = inputSchema;
     if (this.fieldRemover.isPresent()) {
-      return this.fieldRemover.get().removeFields(inputSchema);
+      outputSchema = this.fieldRemover.get().removeFields(inputSchema);
     }
-    return inputSchema;
+    if(workUnit.getPropAsBoolean(ConfigurationKeys.CONVERTER_AVRO_INCLUDE_SCHEMA_CREATION_TIME,
+        ConfigurationKeys.DEFAULT_CONVERTER_AVRO_INCLUDE_SCHEMA_CREATION_TIME)) {
 
 Review comment:
   my concern is that will make the change not backward compatible, and most converter will not erase the schema prop, how about your thoughts?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#discussion_r393765960
 
 

 ##########
 File path: gobblin-core-base/src/main/java/org/apache/gobblin/converter/filter/AvroProjectionConverter.java
 ##########
 @@ -74,10 +74,15 @@ public AvroProjectionConverter init(WorkUnitState workUnit) {
    */
   @Override
   public Schema convertSchema(Schema inputSchema, WorkUnitState workUnit) throws SchemaConversionException {
+    Schema outputSchema = inputSchema;
     if (this.fieldRemover.isPresent()) {
-      return this.fieldRemover.get().removeFields(inputSchema);
+      outputSchema = this.fieldRemover.get().removeFields(inputSchema);
     }
-    return inputSchema;
+    if(workUnit.getPropAsBoolean(ConfigurationKeys.CONVERTER_AVRO_INCLUDE_SCHEMA_CREATION_TIME,
+        ConfigurationKeys.DEFAULT_CONVERTER_AVRO_INCLUDE_SCHEMA_CREATION_TIME)) {
 
 Review comment:
   Given the timestamp is set in the properties of an Avro schema object, does that matter for other writers that are not aware of the existence of this property ? We can try with a simple unit test to see if setting properties will impact writer. 
   
   I mostly concern on the operability problem since there's no guarantee that this converter is and will used everywhere. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] codecov-io edited a comment on issue #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#issuecomment-598522021
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=h1) Report
   > Merging [#2925](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/5796bb4c890cf4c7e48c061907232289cfb08ab9&el=desc) will **decrease** coverage by `0.00%`.
   > The diff coverage is `45.45%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #2925      +/-   ##
   ============================================
   - Coverage     45.46%   45.46%   -0.01%     
   - Complexity     9109     9112       +3     
   ============================================
     Files          1936     1936              
     Lines         73059    73147      +88     
     Branches       8051     8072      +21     
   ============================================
   + Hits          33217    33256      +39     
   - Misses        36782    36819      +37     
   - Partials       3060     3072      +12     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...n/java/org/apache/gobblin/converter/Converter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL0NvbnZlcnRlci5qYXZh) | `74.35% <ø> (ø)` | `11.00 <0.00> (ø)` | |
   | [...c/main/java/org/apache/gobblin/util/AvroUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQXZyb1V0aWxzLmphdmE=) | `56.95% <0.00%> (-0.78%)` | `81.00 <0.00> (ø)` | |
   | [...blin/converter/filter/AvroProjectionConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL2ZpbHRlci9BdnJvUHJvamVjdGlvbkNvbnZlcnRlci5qYXZh) | `77.77% <100.00%> (+2.77%)` | `6.00 <2.00> (ø)` | |
   | [...er/GobblinTrackingEventFlattenFilterConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29udmVydGVyL2ZpbHRlci9Hb2JibGluVHJhY2tpbmdFdmVudEZsYXR0ZW5GaWx0ZXJDb252ZXJ0ZXIuamF2YQ==) | `70.37% <100.00%> (+0.55%)` | `9.00 <0.00> (ø)` | |
   | [...obblin/service/monitoring/FlowStatusGenerator.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9GbG93U3RhdHVzR2VuZXJhdG9yLmphdmE=) | `46.00% <0.00%> (-23.70%)` | `11.00% <0.00%> (ø%)` | |
   | [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXIuamF2YQ==) | `71.33% <0.00%> (-5.14%)` | `13.00% <0.00%> (ø%)` | |
   | [...lin/elasticsearch/writer/FutureCallbackHolder.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tZWxhc3RpY3NlYXJjaC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9lbGFzdGljc2VhcmNoL3dyaXRlci9GdXR1cmVDYWxsYmFja0hvbGRlci5qYXZh) | `61.42% <0.00%> (-1.43%)` | `4.00% <0.00%> (ø%)` | |
   | [...e/gobblin/runtime/locks/ZookeeperBasedJobLock.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbG9ja3MvWm9va2VlcGVyQmFzZWRKb2JMb2NrLmphdmE=) | `63.33% <0.00%> (-1.12%)` | `15.00% <0.00%> (-1.00%)` | |
   | [...he/gobblin/compaction/source/CompactionSource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25Tb3VyY2UuamF2YQ==) | `72.16% <0.00%> (-0.90%)` | `14.00% <0.00%> (ø%)` | |
   | [...main/java/org/apache/gobblin/util/HadoopUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvSGFkb29wVXRpbHMuamF2YQ==) | `30.20% <0.00%> (-0.68%)` | `24.00% <0.00%> (-1.00%)` | |
   | ... and [7 more](https://codecov.io/gh/apache/incubator-gobblin/pull/2925/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=footer). Last update [5796bb4...7a408e0](https://codecov.io/gh/apache/incubator-gobblin/pull/2925?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] asfgit closed pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#discussion_r392464209
 
 

 ##########
 File path: gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java
 ##########
 @@ -309,6 +309,8 @@
   /**
    * Converter configuration properties.
    */
+  public static final String CONVERTER_AVRO_INCLUDE_SCHEMA_CREATION_TIME = "converter.avro.include.schema.creation.time";
 
 Review comment:
   Do we need to make it configurable ? 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] ZihanLi58 commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#discussion_r393228986
 
 

 ##########
 File path: gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java
 ##########
 @@ -309,6 +309,8 @@
   /**
    * Converter configuration properties.
    */
+  public static final String CONVERTER_AVRO_INCLUDE_SCHEMA_CREATION_TIME = "converter.avro.include.schema.creation.time";
 
 Review comment:
   this is only a key name for the config value. Will rename it to avoid confuse.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter

Posted by GitBox <gi...@apache.org>.
autumnust commented on a change in pull request #2925: [GOBBLIN-1080] Add configuration to add schema creation time in converter
URL: https://github.com/apache/incubator-gobblin/pull/2925#discussion_r392466285
 
 

 ##########
 File path: gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java
 ##########
 @@ -833,6 +835,7 @@
   public static final String KAFKA_SCHEMA_REGISTRY_RETRY_TIMES = "kafka.schema.registry.retry.times";
   public static final String KAFKA_SCHEMA_REGISTRY_RETRY_INTERVAL_IN_MILLIS =
       "kafka.schema.registry.retry.interval.inMillis";
+  public static final String SCHEMA_CREATION_TIME_KEY = "CreatedOn";
 
 Review comment:
   If this is supposed to be an internal attribute, you won't need it as public static but should be as close as possible to wherever you are using it. ( I am a bit opposed to adding anything more into ConfigurationKeys but looks like it is common practice that everyone is following. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services