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 2020/03/09 02:07:24 UTC

[GitHub] [incubator-hudi] hddong opened a new pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

hddong opened a new pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388
 
 
   ## What is the purpose of the pull request
   
   *Remove the `EmbeddedTimelineService` from `HoodieReadClient`, so that we can remove `HoodieReadClient` into hudi-spark module*
   
   ## Brief change log
   
     - *Remove embeddedTimelineService from HoodieReadClient*
   
   ## Verify this pull request
   
   This pull request is already covered by existing tests.
   
   ## 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


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io commented on issue #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388#issuecomment-596406596
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1388?src=pr&el=h1) Report
   > Merging [#1388](https://codecov.io/gh/apache/incubator-hudi/pull/1388?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/415882f9023795994e9cc8a8294909bbec7ab191?src=pr&el=desc) will **decrease** coverage by `0.02%`.
   > The diff coverage is `100%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1388/graphs/tree.svg?width=650&token=VTTXabwbs2&height=150&src=pr)](https://codecov.io/gh/apache/incubator-hudi/pull/1388?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1388      +/-   ##
   ============================================
   - Coverage     67.19%   67.17%   -0.03%     
     Complexity      223      223              
   ============================================
     Files           335      335              
     Lines         16279    16272       -7     
     Branches       1661     1661              
   ============================================
   - Hits          10939    10930       -9     
     Misses         4604     4604              
   - Partials        736      738       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1388?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...n/scala/org/apache/hudi/HoodieSparkSqlWriter.scala](https://codecov.io/gh/apache/incubator-hudi/pull/1388/diff?src=pr&el=tree#diff-aHVkaS1zcGFyay9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2h1ZGkvSG9vZGllU3BhcmtTcWxXcml0ZXIuc2NhbGE=) | `52.79% <100%> (-0.87%)` | `0 <0> (ø)` | |
   | [.../java/org/apache/hudi/client/HoodieReadClient.java](https://codecov.io/gh/apache/incubator-hudi/pull/1388/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY2xpZW50L0hvb2RpZVJlYWRDbGllbnQuamF2YQ==) | `100% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...apache/hudi/utilities/deltastreamer/DeltaSync.java](https://codecov.io/gh/apache/incubator-hudi/pull/1388/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvRGVsdGFTeW5jLmphdmE=) | `72.41% <100%> (ø)` | `38 <0> (ø)` | :arrow_down: |
   | [...src/main/java/org/apache/hudi/DataSourceUtils.java](https://codecov.io/gh/apache/incubator-hudi/pull/1388/diff?src=pr&el=tree#diff-aHVkaS1zcGFyay9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaHVkaS9EYXRhU291cmNlVXRpbHMuamF2YQ==) | `50.56% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...n/java/org/apache/hudi/common/model/HoodieKey.java](https://codecov.io/gh/apache/incubator-hudi/pull/1388/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL21vZGVsL0hvb2RpZUtleS5qYXZh) | `88.88% <0%> (-5.56%)` | `0% <0%> (ø)` | |
   | [...a/org/apache/hudi/common/util/collection/Pair.java](https://codecov.io/gh/apache/incubator-hudi/pull/1388/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3V0aWwvY29sbGVjdGlvbi9QYWlyLmphdmE=) | `72% <0%> (-4%)` | `0% <0%> (ø)` | |
   | [...a/org/apache/hudi/client/AbstractHoodieClient.java](https://codecov.io/gh/apache/incubator-hudi/pull/1388/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY2xpZW50L0Fic3RyYWN0SG9vZGllQ2xpZW50LmphdmE=) | `76.31% <0%> (-2.64%)` | `0% <0%> (ø)` | |
   | [...e/hudi/common/table/log/HoodieLogFormatWriter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1388/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL2xvZy9Ib29kaWVMb2dGb3JtYXRXcml0ZXIuamF2YQ==) | `76.92% <0%> (+0.96%)` | `0% <0%> (ø)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1388?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-hudi/pull/1388?src=pr&el=footer). Last update [415882f...929f815](https://codecov.io/gh/apache/incubator-hudi/pull/1388?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-hudi] yanghua commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388#discussion_r390047183
 
 

 ##########
 File path: hudi-client/src/main/java/org/apache/hudi/client/HoodieReadClient.java
 ##########
 @@ -100,14 +91,6 @@ public HoodieReadClient(JavaSparkContext jsc, String basePath, SQLContext sqlCon
    * @param clientConfig instance of HoodieWriteConfig
    */
   public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig) {
-    this(jsc, clientConfig, Option.empty());
-  }
-
-  /**
-   * @param clientConfig instance of HoodieWriteConfig
-   */
-  public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig,
-      Option<EmbeddedTimelineService> timelineService) {
 
 Review comment:
   @bvaradar I and @vinothchandar discussed before, we agreed on that `HoodieReadClient ` will be moved into hudi-spark module in the future. Currently, we found `EmbeddedTimelineService` did not take effect in `HoodieReadClient`.
   
   About making `EmbeddedTimelineService` spark-free, I have filed an issue to track this work: https://issues.apache.org/jira/browse/HUDI-661

----------------------------------------------------------------
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-hudi] vinothchandar commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388#discussion_r389478969
 
 

 ##########
 File path: hudi-client/src/main/java/org/apache/hudi/client/HoodieReadClient.java
 ##########
 @@ -100,14 +91,6 @@ public HoodieReadClient(JavaSparkContext jsc, String basePath, SQLContext sqlCon
    * @param clientConfig instance of HoodieWriteConfig
    */
   public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig) {
-    this(jsc, clientConfig, Option.empty());
-  }
-
-  /**
-   * @param clientConfig instance of HoodieWriteConfig
-   */
-  public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig,
-      Option<EmbeddedTimelineService> timelineService) {
 
 Review comment:
   so this was not doing anything after we changed the class hierrarchy? and this PR is cleaning this up? 

----------------------------------------------------------------
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-hudi] hddong commented on issue #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
hddong commented on issue #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388#issuecomment-596394851
 
 
   @yanghua Passed the test in the local environment, try to rebuild.

----------------------------------------------------------------
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-hudi] bvaradar commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
bvaradar commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388#discussion_r389797461
 
 

 ##########
 File path: hudi-client/src/main/java/org/apache/hudi/client/HoodieReadClient.java
 ##########
 @@ -100,14 +91,6 @@ public HoodieReadClient(JavaSparkContext jsc, String basePath, SQLContext sqlCon
    * @param clientConfig instance of HoodieWriteConfig
    */
   public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig) {
-    this(jsc, clientConfig, Option.empty());
-  }
-
-  /**
-   * @param clientConfig instance of HoodieWriteConfig
-   */
-  public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig,
-      Option<EmbeddedTimelineService> timelineService) {
 
 Review comment:
   Thanks @vinothchandar  for pointing me to this change. @yanghua : Just catching up on this. Instead of completely removing EmbeddedTimelineService from HoodieReadClient, did you try removing SparkConf dependency from  EmbeddedTimelineService thereby also making it generic. I wanted to make sure all APIs of hudi client go through Embedded timeline service so that we can guarantee meaningful advantage when using timeline service in future. 

----------------------------------------------------------------
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-hudi] vinothchandar commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388#discussion_r389480134
 
 

 ##########
 File path: hudi-client/src/main/java/org/apache/hudi/client/HoodieReadClient.java
 ##########
 @@ -100,14 +91,6 @@ public HoodieReadClient(JavaSparkContext jsc, String basePath, SQLContext sqlCon
    * @param clientConfig instance of HoodieWriteConfig
    */
   public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig) {
-    this(jsc, clientConfig, Option.empty());
-  }
-
-  /**
-   * @param clientConfig instance of HoodieWriteConfig
-   */
-  public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig,
-      Option<EmbeddedTimelineService> timelineService) {
 
 Review comment:
   Okay sg. its simpler for now..  cc @bvaradar as fyi.. you may run into this if interested in de-duping.. 

----------------------------------------------------------------
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-hudi] yanghua merged pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
yanghua merged pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388
 
 
   

----------------------------------------------------------------
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-hudi] bvaradar commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
bvaradar commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388#discussion_r390079655
 
 

 ##########
 File path: hudi-client/src/main/java/org/apache/hudi/client/HoodieReadClient.java
 ##########
 @@ -100,14 +91,6 @@ public HoodieReadClient(JavaSparkContext jsc, String basePath, SQLContext sqlCon
    * @param clientConfig instance of HoodieWriteConfig
    */
   public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig) {
-    this(jsc, clientConfig, Option.empty());
-  }
-
-  /**
-   * @param clientConfig instance of HoodieWriteConfig
-   */
-  public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig,
-      Option<EmbeddedTimelineService> timelineService) {
 
 Review comment:
   @yanghua : I am ok as long as with HUDI-661, we are bringing back TimelineService support for ReadClient. We need to make sure HUDI-661 is also addressed quickly before next release (0.6.0).  I understand that refactoring these pieces is sometimes tricky and am ok with your approach. 

----------------------------------------------------------------
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-hudi] yanghua commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388#discussion_r390107667
 
 

 ##########
 File path: hudi-client/src/main/java/org/apache/hudi/client/HoodieReadClient.java
 ##########
 @@ -100,14 +91,6 @@ public HoodieReadClient(JavaSparkContext jsc, String basePath, SQLContext sqlCon
    * @param clientConfig instance of HoodieWriteConfig
    */
   public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig) {
-    this(jsc, clientConfig, Option.empty());
-  }
-
-  /**
-   * @param clientConfig instance of HoodieWriteConfig
-   */
-  public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig,
-      Option<EmbeddedTimelineService> timelineService) {
 
 Review comment:
   @bvaradar Will try to return back soon, I am helping to release v0.5.2.

----------------------------------------------------------------
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-hudi] yanghua commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388#discussion_r389479292
 
 

 ##########
 File path: hudi-client/src/main/java/org/apache/hudi/client/HoodieReadClient.java
 ##########
 @@ -100,14 +91,6 @@ public HoodieReadClient(JavaSparkContext jsc, String basePath, SQLContext sqlCon
    * @param clientConfig instance of HoodieWriteConfig
    */
   public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig) {
-    this(jsc, clientConfig, Option.empty());
-  }
-
-  /**
-   * @param clientConfig instance of HoodieWriteConfig
-   */
-  public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig,
-      Option<EmbeddedTimelineService> timelineService) {
 
 Review comment:
   Yes

----------------------------------------------------------------
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-hudi] yanghua commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1388: [HUDI-681]Remove embeddedTimelineService from HoodieReadClient
URL: https://github.com/apache/incubator-hudi/pull/1388#discussion_r390047183
 
 

 ##########
 File path: hudi-client/src/main/java/org/apache/hudi/client/HoodieReadClient.java
 ##########
 @@ -100,14 +91,6 @@ public HoodieReadClient(JavaSparkContext jsc, String basePath, SQLContext sqlCon
    * @param clientConfig instance of HoodieWriteConfig
    */
   public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig) {
-    this(jsc, clientConfig, Option.empty());
-  }
-
-  /**
-   * @param clientConfig instance of HoodieWriteConfig
-   */
-  public HoodieReadClient(JavaSparkContext jsc, HoodieWriteConfig clientConfig,
-      Option<EmbeddedTimelineService> timelineService) {
 
 Review comment:
   @bvaradar I and @vinothchandar discussed before, we agreed on that `HoodieReadClient ` will be moved into hudi-spark module in the future. Currently, we found {{EmbeddedTimelineService}} did not take effect in `HoodieReadClient`.

----------------------------------------------------------------
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