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