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/06/09 17:58:18 UTC
[GitHub] [hudi] EdwinGuo opened a new pull request #1721: Cache the explodeRecordRDDWithFileComparisons instead of commuting it…
EdwinGuo opened a new pull request #1721:
URL: https://github.com/apache/hudi/pull/1721
… twice in lookUpIndex
## *Tips*
- *Thank you very much for contributing to Apache Hudi.*
- *Please review https://hudi.apache.org/contributing.html before opening a pull request.*
## What is the purpose of the pull request
Cache the explodeRecordRDDWithFileComparisons instead of commuting it twice in lookUpIndex
*(For example: This pull request adds quick-start document.)*
## Brief change log
*(for example:)*
- *Modify AnnotationLocation checkstyle rule in checkstyle.xml*
## Verify this pull request
*(Please pick either of the following options)*
This pull request is already covered by existing tests, such as *(please describe tests)*.
(or)
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end.*
- *Added HoodieClientWriteTest to verify the change.*
- *Manually verified the change by running a job locally.*
## 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] vinothchandar commented on pull request #1721: [WIP] [HUDI-1041] Cache the explodeRecordRDDWithFileComparisons instead of commuting it…
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1721:
URL: https://github.com/apache/hudi/pull/1721#issuecomment-914630943
Closing due to inactivity
--
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.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] EdwinGuo commented on pull request #1721: [WIP] [HUDI-1041] Cache the explodeRecordRDDWithFileComparisons instead of commuting it…
Posted by GitBox <gi...@apache.org>.
EdwinGuo commented on pull request #1721:
URL: https://github.com/apache/hudi/pull/1721#issuecomment-648532688
> @EdwinGuo @nsivabalan let's hash this out.. its an interesting one.. Although it may seem like we are computing the fully exploded RDD in both places.. if you look closely, we do
>
> ```
> fileToComparisons = explodeRecordRDDWithFileComparisons(partitionToFileInfo, partitionRecordKeyPairRDD)
> .mapToPair(t -> t).countByKey();
> ```
>
> countByKey() does not shuffle actual data, but just the counts per file.. We only pay the compute cost of exploding twice.. And all this to estimate the parallelism. given this is jsut an estimate, would it be better to introduce an option to simply down sample and estimate, rather than adding caching?
>
> eg.
>
> ```
> fileToComparisons = explodeRecordRDDWithFileComparisons(partitionToFileInfo, partitionRecordKeyPairRDD.sample(true, 0.1))
> .mapToPair(t -> t).countByKey();
> ```
>
> would cut the cost down by 90% .. we need to adjust the computations in the map accordingly ofc..
> Even spark sort does some kind of reservoir sampling.. So this could be a valid approach overall.
>
> What do you both think? I am bit concerned about caching this exploded RDD (that's why I chose to recompute to begin with)
I can provide some performance comparison tomorrow. fileComparisonsRDD is being compute in a different patterns within findMatchingFilesForRecordKeys and computeComparisonsPerFileGroup, so yes, countByKey is light in shuffle but could be heavy in IO for some of the cases. I agree StorageLevel.MEMORY_AND_DISK_SER() could be heavy than recompute in some of the scenario, so let me conduct some performance testing and get back to you.
Regarding sampling, what if some of the partitions are skewed? Will that cause more overhead than flush the file out?
----------------------------------------------------------------
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] EdwinGuo commented on pull request #1721: [WIP] [HUDI-1041] Cache the explodeRecordRDDWithFileComparisons instead of commuting it…
Posted by GitBox <gi...@apache.org>.
EdwinGuo commented on pull request #1721:
URL: https://github.com/apache/hudi/pull/1721#issuecomment-648529008
> Can you please include the jira number in the pr title
Done. https://issues.apache.org/jira/browse/HUDI-1041
----------------------------------------------------------------
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-commenter commented on pull request #1721: Cache the explodeRecordRDDWithFileComparisons instead of commuting it…
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1721:
URL: https://github.com/apache/hudi/pull/1721#issuecomment-641622744
# [Codecov](https://codecov.io/gh/apache/hudi/pull/1721?src=pr&el=h1) Report
> Merging [#1721](https://codecov.io/gh/apache/hudi/pull/1721?src=pr&el=desc) into [master](https://codecov.io/gh/apache/hudi/commit/37838cea6094ddc66191df42e8b2c84f132d1623&el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/hudi/pull/1721?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1721 +/- ##
============================================
- Coverage 18.16% 18.15% -0.01%
Complexity 860 860
============================================
Files 352 352
Lines 15411 15410 -1
Branches 1525 1524 -1
============================================
- Hits 2799 2798 -1
Misses 12254 12254
Partials 358 358
```
| [Impacted Files](https://codecov.io/gh/apache/hudi/pull/1721?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../org/apache/hudi/index/bloom/HoodieBloomIndex.java](https://codecov.io/gh/apache/hudi/pull/1721/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaW5kZXgvYmxvb20vSG9vZGllQmxvb21JbmRleC5qYXZh) | `58.16% <100.00%> (-0.43%)` | `16.00 <0.00> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/hudi/pull/1721?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/hudi/pull/1721?src=pr&el=footer). Last update [37838ce...ef08b1e](https://codecov.io/gh/apache/hudi/pull/1721?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
[GitHub] [hudi] codecov-commenter edited a comment on pull request #1721: Cache the explodeRecordRDDWithFileComparisons instead of commuting it…
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1721:
URL: https://github.com/apache/hudi/pull/1721#issuecomment-641622744
# [Codecov](https://codecov.io/gh/apache/hudi/pull/1721?src=pr&el=h1) Report
> Merging [#1721](https://codecov.io/gh/apache/hudi/pull/1721?src=pr&el=desc) into [master](https://codecov.io/gh/apache/hudi/commit/37838cea6094ddc66191df42e8b2c84f132d1623&el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/hudi/pull/1721?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #1721 +/- ##
============================================
- Coverage 18.16% 18.15% -0.01%
Complexity 860 860
============================================
Files 352 352
Lines 15411 15410 -1
Branches 1525 1524 -1
============================================
- Hits 2799 2798 -1
Misses 12254 12254
Partials 358 358
```
| [Impacted Files](https://codecov.io/gh/apache/hudi/pull/1721?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../org/apache/hudi/index/bloom/HoodieBloomIndex.java](https://codecov.io/gh/apache/hudi/pull/1721/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaW5kZXgvYmxvb20vSG9vZGllQmxvb21JbmRleC5qYXZh) | `58.16% <100.00%> (-0.43%)` | `16.00 <0.00> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/hudi/pull/1721?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/hudi/pull/1721?src=pr&el=footer). Last update [37838ce...ef08b1e](https://codecov.io/gh/apache/hudi/pull/1721?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
[GitHub] [hudi] EdwinGuo commented on pull request #1721: Cache the explodeRecordRDDWithFileComparisons instead of commuting it…
Posted by GitBox <gi...@apache.org>.
EdwinGuo commented on pull request #1721:
URL: https://github.com/apache/hudi/pull/1721#issuecomment-642922843
> Good one. If you don't mind, can you run a sample job(with 1 M records or something) and show the spark UI stages screen shot to see the difference with and w/o this optimization.
Will do. Thanks for reviewing.
----------------------------------------------------------------
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 closed pull request #1721: [WIP] [HUDI-1041] Cache the explodeRecordRDDWithFileComparisons instead of commuting it…
Posted by GitBox <gi...@apache.org>.
vinothchandar closed pull request #1721:
URL: https://github.com/apache/hudi/pull/1721
--
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.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] vinothchandar commented on pull request #1721: [WIP] [HUDI-1041] Cache the explodeRecordRDDWithFileComparisons instead of commuting it…
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1721:
URL: https://github.com/apache/hudi/pull/1721#issuecomment-648885633
> Regarding sampling, what if some of the partitions are skewed? Will that cause more overhead than flush the file out?
IIRC the partitionRecordKeyPairRDD would have even distribution of keys from the precombine step which just does a `reduceByKey`. We can always support a config to increase the sampling rate, right? All depends on how much difference there is in the computed parallelism with samplingRate=0.1 and 1.0?
----------------------------------------------------------------
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 #1721: Cache the explodeRecordRDDWithFileComparisons instead of commuting it…
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1721:
URL: https://github.com/apache/hudi/pull/1721#issuecomment-647825299
Can you please include the jira number in the pr title
----------------------------------------------------------------
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