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/19 10:12:47 UTC
[GitHub] [hudi] yanghua opened a new pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
yanghua opened a new pull request #2458:
URL: https://github.com/apache/hudi/pull/2458
## What is the purpose of the pull request
*This pull request renames `FileSystemViewHandler` to `Router` and corrected the class comment*
## Brief change log
- *Rename FileSystemViewHandler to Router and corrected the class comment*
## Verify this pull request
This pull request is a trivial rework / code cleanup without any test coverage.
## Committer checklist
- [ ] Has a corresponding JIRA in PR title & commit
- [ ] Commit message is descriptive of the change
- [ ] CI is green
- [ ] Necessary doc changes done or have another open PR
- [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
----------------------------------------------------------------
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] yanghua commented on pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-771541383
> I will let you make the RequestHandler vs Router call. and land.
`RequestHandler` sounds better.
----------------------------------------------------------------
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 #2458: [MINOR] Rename FileSystemViewHandler to RequestHandler and corrected the class comment
Posted by GitBox <gi...@apache.org>.
vinothchandar merged pull request #2458:
URL: https://github.com/apache/hudi/pull/2458
----------------------------------------------------------------
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] yanghua commented on pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-762893505
> @yanghua Sorry that I am not fully get the point here. Handler should be ok here?
It should be a router to route all the request uri to the mapped handlers. Here is the registry: https://github.com/apache/hudi/pull/2458/files#diff-df28a035185ac88dc61c2b8c310225cf4bed83803f671f73d8b8a37c08d8392bR74
IMO, the real handlers all extend `Handler`.
----------------------------------------------------------------
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 #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-762818490
# [Codecov](https://codecov.io/gh/apache/hudi/pull/2458?src=pr&el=h1) Report
> Merging [#2458](https://codecov.io/gh/apache/hudi/pull/2458?src=pr&el=desc) (c202334) into [master](https://codecov.io/gh/apache/hudi/commit/a38612b10f6ae04644519270f9b5eb631a77c148?el=desc) (a38612b) will **decrease** coverage by `41.00%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/hudi/pull/2458?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #2458 +/- ##
============================================
- Coverage 50.69% 9.68% -41.01%
+ Complexity 3059 48 -3011
============================================
Files 419 53 -366
Lines 18810 1930 -16880
Branches 1924 230 -1694
============================================
- Hits 9535 187 -9348
+ Misses 8498 1730 -6768
+ Partials 777 13 -764
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| hudicli | `?` | `?` | |
| hudiclient | `?` | `?` | |
| hudicommon | `?` | `?` | |
| hudiflink | `?` | `?` | |
| hudihadoopmr | `?` | `?` | |
| hudisparkdatasource | `?` | `?` | |
| hudisync | `?` | `?` | |
| huditimelineservice | `?` | `?` | |
| hudiutilities | `9.68% <ø> (-59.80%)` | `0.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/2458?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...va/org/apache/hudi/utilities/IdentitySplitter.java](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL0lkZW50aXR5U3BsaXR0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
| [...va/org/apache/hudi/utilities/schema/SchemaSet.java](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NjaGVtYS9TY2hlbWFTZXQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
| [...a/org/apache/hudi/utilities/sources/RowSource.java](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvUm93U291cmNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-4.00%)` | |
| [.../org/apache/hudi/utilities/sources/AvroSource.java](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvQXZyb1NvdXJjZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [.../org/apache/hudi/utilities/sources/JsonSource.java](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvSnNvblNvdXJjZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
| [...rg/apache/hudi/utilities/sources/CsvDFSSource.java](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvQ3N2REZTU291cmNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-10.00%)` | |
| [...g/apache/hudi/utilities/sources/JsonDFSSource.java](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvSnNvbkRGU1NvdXJjZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-4.00%)` | |
| [...apache/hudi/utilities/sources/JsonKafkaSource.java](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvSnNvbkthZmthU291cmNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-6.00%)` | |
| [...pache/hudi/utilities/sources/ParquetDFSSource.java](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NvdXJjZXMvUGFycXVldERGU1NvdXJjZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-5.00%)` | |
| [...lities/schema/SchemaProviderWithPostProcessor.java](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL3NjaGVtYS9TY2hlbWFQcm92aWRlcldpdGhQb3N0UHJvY2Vzc29yLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-4.00%)` | |
| ... and [394 more](https://codecov.io/gh/apache/hudi/pull/2458/diff?src=pr&el=tree-more) | |
----------------------------------------------------------------
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 pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-762897273
I get where you are coming from. But I find Router kind of confusing. Its typical to name such classes that take all requests as handlers right? May be just FileSystemViewRequestHandler? I suggest we not remove FileSystemView part of the name. Its a core abstraction and good to preserve that connection. Wdyt?
----------------------------------------------------------------
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 #2458: [MINOR] Rename FileSystemViewHandler to RequestHandler and corrected the class comment
Posted by GitBox <gi...@apache.org>.
vinothchandar merged pull request #2458:
URL: https://github.com/apache/hudi/pull/2458
----------------------------------------------------------------
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] yanghua commented on pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-762902669
> I get where you are coming from. But I find Router kind of confusing. Its typical to name such classes that take all requests as handlers right? May be just FileSystemViewRequestHandler? I suggest we not remove FileSystemView part of the name. Its a core abstraction and good to preserve that connection. Wdyt?
Perhaps different understandings depend on what we think this class will assume in the future and the boundaries of services. I want to change its name because it seems to be easily confused with `BaseFileHandler`. People who don't know the details will think that they are the same type of handler, but it is not.
----------------------------------------------------------------
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] yanghua edited a comment on pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
yanghua edited a comment on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-762902669
> I get where you are coming from. But I find Router kind of confusing. Its typical to name such classes that take all requests as handlers right? May be just FileSystemViewRequestHandler? Its a core abstraction and good to preserve that connection. Wdyt?
Perhaps different understandings depend on what we think this class will assume in the future and the boundaries of services. I want to change its name because it seems to be easily confused with `BaseFileHandler`. People who don't know the details will think that they are the same type of handler, but it is not.
This module used to provide web services or RESTful API for timeline. It has many handlers (more in the future?) and one handler-registry(URI mapping, or say router)which is easy to understand?
> I suggest we not remove FileSystemView part of the name.
Currently, we can keep it. But when we provide more services in the future, maybe it will be a bit limited?
However, I do think that if this is the `hudi-web-ui` or `hudi-rest-api` module, it is more appropriate to call it router.
----------------------------------------------------------------
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] yanghua commented on pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
yanghua commented on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-771541383
> I will let you make the RequestHandler vs Router call. and land.
`RequestHandler` sounds better.
----------------------------------------------------------------
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] yanghua edited a comment on pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
yanghua edited a comment on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-762902669
> I get where you are coming from. But I find Router kind of confusing. Its typical to name such classes that take all requests as handlers right? May be just FileSystemViewRequestHandler? Its a core abstraction and good to preserve that connection. Wdyt?
Perhaps different understandings depend on what we think this class will assume in the future and the boundaries of services. I want to change its name because it seems to be easily confused with `BaseFileHandler`. People who don't know the details will think that they are the same type of handler, but it is not.
This module used to provide web services or RESTful API for timeline. It has many handlers (more in the future?) and one handler-registry(URI mapping, or say router)which is easy to understand?
> I suggest we not remove FileSystemView part of the name.
Currently, we can keep it. But when we provide more services in the future, maybe it will be a bit limited?
----------------------------------------------------------------
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] yanghua edited a comment on pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
yanghua edited a comment on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-762893505
> @yanghua Sorry that I am not fully get the point here. Handler should be ok here?
It should be a router to route all the request uri to the mapped handlers. Here is the registry: https://github.com/apache/hudi/blob/c2023342eb92c0f54b4bb958a6284e065eadc8bc/hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/Router.java#L74
IMO, the real handlers all extend `Handler`.
----------------------------------------------------------------
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 pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-771044188
>Currently, we can keep it. But when we provide more services in the future, maybe it will be a bit limited?
Agree. We can rename as we add more and more. To clarify, I was mostly disagreeing with the `Router` vs `RequestHandler`.
I find `RequestHandler` in line with how rest request handlers are named (at least from code I have seen).
On the `FileSystemView` prefix, I stand corrected. it's serving more than just the filesystem view. So its apt to rename this even now.
Apologies again. Got confused, when dealing with a lot of stuff :)
----------------------------------------------------------------
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] yanghua edited a comment on pull request #2458: [MINOR] Rename FileSystemViewHandler to Router and corrected the class comment
Posted by GitBox <gi...@apache.org>.
yanghua edited a comment on pull request #2458:
URL: https://github.com/apache/hudi/pull/2458#issuecomment-762902669
> I get where you are coming from. But I find Router kind of confusing. Its typical to name such classes that take all requests as handlers right? May be just FileSystemViewRequestHandler? I suggest we not remove FileSystemView part of the name. Its a core abstraction and good to preserve that connection. Wdyt?
Perhaps different understandings depend on what we think this class will assume in the future and the boundaries of services. I want to change its name because it seems to be easily confused with `BaseFileHandler`. People who don't know the details will think that they are the same type of handler, but it is not.
This module used to provide web services or RESTful API for timeline. It has many handlers (more in the future?) and one handler-registry(URI mapping, or say router)which is easy to understand?
----------------------------------------------------------------
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