You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2021/01/22 15:59:37 UTC
[GitHub] [hudi] vburenin opened a new pull request #2476: [HUDI-1538] Try to init class trying different signatures instead of checking its name
vburenin opened a new pull request #2476:
URL: https://github.com/apache/hudi/pull/2476
## What is the purpose of the pull request
UtilHelpers.createSource had a hardcoded way of checking which
constructor signature needs to be used to instantiate a class
which makes it impossible to override those hardcoded classes outside of Hudi
and use them instead as their signature remains the same.
## Verify this pull request
This pull request is a trivial rework / code cleanup without any test coverage.
----------------------------------------------------------------
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
[GitHub] [hudi] vinothchandar commented on a change in pull request #2476: [HUDI-1538] Try to init class trying different signatures instead of checking its name
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #2476:
URL: https://github.com/apache/hudi/pull/2476#discussion_r566524170
##########
File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/UtilHelpers.java
##########
@@ -96,19 +94,21 @@
private static final Logger LOG = LogManager.getLogger(UtilHelpers.class);
public static Source createSource(String sourceClass, TypedProperties cfg, JavaSparkContext jssc,
- SparkSession sparkSession, SchemaProvider schemaProvider, HoodieDeltaStreamerMetrics metrics) throws IOException {
-
+ SparkSession sparkSession, SchemaProvider schemaProvider,
+ HoodieDeltaStreamerMetrics metrics) throws IOException {
try {
- if (JsonKafkaSource.class.getName().equals(sourceClass)
- || AvroKafkaSource.class.getName().equals(sourceClass)) {
+ try {
Review comment:
I am not sure if handling this via exception is making this more readable. wdyt?
For e.g the current impl makes it clear, in what case a certain constructor is called,.
----------------------------------------------------------------
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
[GitHub] [hudi] codecov-io edited a comment on pull request #2476: [HUDI-1538] Try to init class trying different signatures instead of checking its name
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #2476:
URL: https://github.com/apache/hudi/pull/2476#issuecomment-765617591
# [Codecov](https://codecov.io/gh/apache/hudi/pull/2476?src=pr&el=h1) Report
> Merging [#2476](https://codecov.io/gh/apache/hudi/pull/2476?src=pr&el=desc) (d504a97) into [master](https://codecov.io/gh/apache/hudi/commit/048633da1a913a05252b1b5dea0b3d40d75c81b4?el=desc) (048633d) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/hudi/pull/2476?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2476 +/- ##
============================================
+ Coverage 50.17% 50.18% +0.01%
+ Complexity 3050 3049 -1
============================================
Files 419 419
Lines 18931 18930 -1
Branches 1948 1947 -1
============================================
+ Hits 9498 9500 +2
+ Misses 8657 8656 -1
+ Partials 776 774 -2
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| hudicli | `37.21% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudiclient | `100.00% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudicommon | `51.49% <ø> (+0.02%)` | `0.00 <ø> (ø)` | |
| hudiflink | `0.00% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudihadoopmr | `33.16% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudisparkdatasource | `65.85% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudisync | `48.61% <ø> (ø)` | `0.00 <ø> (ø)` | |
| huditimelineservice | `66.49% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudiutilities | `69.46% <100.00%> (+0.03%)` | `0.00 <1.00> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/hudi/pull/2476?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...in/java/org/apache/hudi/utilities/UtilHelpers.java](https://codecov.io/gh/apache/hudi/pull/2476/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL1V0aWxIZWxwZXJzLmphdmE=) | `64.53% <100.00%> (+0.37%)` | `32.00 <1.00> (-1.00)` | :arrow_up: |
| [...e/hudi/common/table/log/HoodieLogFormatWriter.java](https://codecov.io/gh/apache/hudi/pull/2476/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL2xvZy9Ib29kaWVMb2dGb3JtYXRXcml0ZXIuamF2YQ==) | `79.68% <0.00%> (+1.56%)` | `26.00% <0.00%> (ø%)` | |
----------------------------------------------------------------
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
[GitHub] [hudi] vburenin commented on pull request #2476: [HUDI-1538] Try to init class trying different signatures instead of checking its name
Posted by GitBox <gi...@apache.org>.
vburenin commented on pull request #2476:
URL: https://github.com/apache/hudi/pull/2476#issuecomment-765508771
@yanghua This is a new PR that is based on not diverted master branch.
----------------------------------------------------------------
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
[GitHub] [hudi] vburenin commented on pull request #2476: [HUDI-1538] Try to init class trying different signatures instead of checking its name
Posted by GitBox <gi...@apache.org>.
vburenin commented on pull request #2476:
URL: https://github.com/apache/hudi/pull/2476#issuecomment-766947415
Can anybody merge this PR, please?
----------------------------------------------------------------
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
[GitHub] [hudi] vburenin commented on pull request #2476: [HUDI-1538] Try to init class trying different signatures instead of checking its name
Posted by GitBox <gi...@apache.org>.
vburenin commented on pull request #2476:
URL: https://github.com/apache/hudi/pull/2476#issuecomment-766947415
Can anybody merge this PR, please?
----------------------------------------------------------------
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
[GitHub] [hudi] vburenin commented on a change in pull request #2476: [HUDI-1538] Try to init class trying different signatures instead of checking its name
Posted by GitBox <gi...@apache.org>.
vburenin commented on a change in pull request #2476:
URL: https://github.com/apache/hudi/pull/2476#discussion_r566977565
##########
File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/UtilHelpers.java
##########
@@ -96,19 +94,21 @@
private static final Logger LOG = LogManager.getLogger(UtilHelpers.class);
public static Source createSource(String sourceClass, TypedProperties cfg, JavaSparkContext jssc,
- SparkSession sparkSession, SchemaProvider schemaProvider, HoodieDeltaStreamerMetrics metrics) throws IOException {
-
+ SparkSession sparkSession, SchemaProvider schemaProvider,
+ HoodieDeltaStreamerMetrics metrics) throws IOException {
try {
- if (JsonKafkaSource.class.getName().equals(sourceClass)
- || AvroKafkaSource.class.getName().equals(sourceClass)) {
+ try {
Review comment:
Not sure about readability, I would say it looks fine to me. The goal is to add an ability to override the parent class that expects "metrics" as a parameter, otherwise I simply can't do that and I can't include that class into the hudi source.
----------------------------------------------------------------
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
[GitHub] [hudi] vinothchandar merged pull request #2476: [HUDI-1538] Try to init class trying different signatures instead of checking its name
Posted by GitBox <gi...@apache.org>.
vinothchandar merged pull request #2476:
URL: https://github.com/apache/hudi/pull/2476
----------------------------------------------------------------
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
[GitHub] [hudi] vinothchandar commented on a change in pull request #2476: [HUDI-1538] Try to init class trying different signatures instead of checking its name
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #2476:
URL: https://github.com/apache/hudi/pull/2476#discussion_r569725483
##########
File path: hudi-utilities/src/main/java/org/apache/hudi/utilities/UtilHelpers.java
##########
@@ -96,19 +94,21 @@
private static final Logger LOG = LogManager.getLogger(UtilHelpers.class);
public static Source createSource(String sourceClass, TypedProperties cfg, JavaSparkContext jssc,
- SparkSession sparkSession, SchemaProvider schemaProvider, HoodieDeltaStreamerMetrics metrics) throws IOException {
-
+ SparkSession sparkSession, SchemaProvider schemaProvider,
+ HoodieDeltaStreamerMetrics metrics) throws IOException {
try {
- if (JsonKafkaSource.class.getName().equals(sourceClass)
- || AvroKafkaSource.class.getName().equals(sourceClass)) {
+ try {
Review comment:
Okay my bad. I understand what you tried to do now. So you have your own source class, that implements has a constructor with the metrics argument.
I was thinking along the lines of having all sources take in the metrics field. We can deal with that separately
----------------------------------------------------------------
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
[GitHub] [hudi] codecov-io commented on pull request #2476: [HUDI-1538] Try to init class trying different signatures instead of checking its name
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #2476:
URL: https://github.com/apache/hudi/pull/2476#issuecomment-765617591
# [Codecov](https://codecov.io/gh/apache/hudi/pull/2476?src=pr&el=h1) Report
> Merging [#2476](https://codecov.io/gh/apache/hudi/pull/2476?src=pr&el=desc) (d504a97) into [master](https://codecov.io/gh/apache/hudi/commit/048633da1a913a05252b1b5dea0b3d40d75c81b4?el=desc) (048633d) will **decrease** coverage by `0.32%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/hudi/pull/2476?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2476 +/- ##
============================================
- Coverage 50.17% 49.84% -0.33%
+ Complexity 3050 2989 -61
============================================
Files 419 413 -6
Lines 18931 18545 -386
Branches 1948 1930 -18
============================================
- Hits 9498 9244 -254
+ Misses 8657 8541 -116
+ Partials 776 760 -16
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| hudicli | `37.21% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudiclient | `100.00% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudicommon | `51.49% <ø> (+0.02%)` | `0.00 <ø> (ø)` | |
| hudiflink | `0.00% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudihadoopmr | `33.16% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudisparkdatasource | `65.85% <ø> (ø)` | `0.00 <ø> (ø)` | |
| hudisync | `48.61% <ø> (ø)` | `0.00 <ø> (ø)` | |
| huditimelineservice | `?` | `?` | |
| hudiutilities | `69.46% <100.00%> (+0.03%)` | `0.00 <1.00> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/hudi/pull/2476?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...in/java/org/apache/hudi/utilities/UtilHelpers.java](https://codecov.io/gh/apache/hudi/pull/2476/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL1V0aWxIZWxwZXJzLmphdmE=) | `64.53% <100.00%> (+0.37%)` | `32.00 <1.00> (-1.00)` | :arrow_up: |
| [...udi/timeline/service/handlers/BaseFileHandler.java](https://codecov.io/gh/apache/hudi/pull/2476/diff?src=pr&el=tree#diff-aHVkaS10aW1lbGluZS1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL3RpbWVsaW5lL3NlcnZpY2UvaGFuZGxlcnMvQmFzZUZpbGVIYW5kbGVyLmphdmE=) | | | |
| [...apache/hudi/timeline/service/handlers/Handler.java](https://codecov.io/gh/apache/hudi/pull/2476/diff?src=pr&el=tree#diff-aHVkaS10aW1lbGluZS1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL3RpbWVsaW5lL3NlcnZpY2UvaGFuZGxlcnMvSGFuZGxlci5qYXZh) | | | |
| [.../apache/hudi/timeline/service/TimelineService.java](https://codecov.io/gh/apache/hudi/pull/2476/diff?src=pr&el=tree#diff-aHVkaS10aW1lbGluZS1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL3RpbWVsaW5lL3NlcnZpY2UvVGltZWxpbmVTZXJ2aWNlLmphdmE=) | | | |
| [...udi/timeline/service/handlers/TimelineHandler.java](https://codecov.io/gh/apache/hudi/pull/2476/diff?src=pr&el=tree#diff-aHVkaS10aW1lbGluZS1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL3RpbWVsaW5lL3NlcnZpY2UvaGFuZGxlcnMvVGltZWxpbmVIYW5kbGVyLmphdmE=) | | | |
| [...e/hudi/timeline/service/FileSystemViewHandler.java](https://codecov.io/gh/apache/hudi/pull/2476/diff?src=pr&el=tree#diff-aHVkaS10aW1lbGluZS1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL3RpbWVsaW5lL3NlcnZpY2UvRmlsZVN5c3RlbVZpZXdIYW5kbGVyLmphdmE=) | | | |
| [...di/timeline/service/handlers/FileSliceHandler.java](https://codecov.io/gh/apache/hudi/pull/2476/diff?src=pr&el=tree#diff-aHVkaS10aW1lbGluZS1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL3RpbWVsaW5lL3NlcnZpY2UvaGFuZGxlcnMvRmlsZVNsaWNlSGFuZGxlci5qYXZh) | | | |
| [...e/hudi/common/table/log/HoodieLogFormatWriter.java](https://codecov.io/gh/apache/hudi/pull/2476/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL2xvZy9Ib29kaWVMb2dGb3JtYXRXcml0ZXIuamF2YQ==) | `79.68% <0.00%> (+1.56%)` | `26.00% <0.00%> (ø%)` | |
----------------------------------------------------------------
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