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 2019/12/24 05:24:36 UTC
[GitHub] [incubator-hudi] cdmikechen opened a new pull request #1126: - Fix
Error: java.lang.IllegalArgumentException: Can not create a Path from an
empty string
cdmikechen opened a new pull request #1126: - Fix Error: java.lang.IllegalArgumentException: Can not create a Path from an empty string
URL: https://github.com/apache/incubator-hudi/pull/1126
same link in https://github.com/apache/incubator-hudi/pull/771
this time is `deleteFilesFunc` method in `org.apache.hudi.table.HoodieCopyOnWrite.java`.
## *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
Fix Error: java.lang.IllegalArgumentException: Can not create a Path from an empty string, when upsert a hudi table with non-partition data.
## Brief change log
change `HoodieCopyOnWriteTable` in line 112 use `FSUtils.getPartitionPath(Path basePath, String partitionPath)`
## Verify this pull request
This pull request is already covered by existing tests, such as *org.apache.hudi.table.TestCopyOnWriteTable*.
## Committer checklist
- [ ] Has a corresponding JIRA in PR title & commit
- [x] 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] cdmikechen commented on a change in pull request
#1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path
from an empty string
Posted by GitBox <gi...@apache.org>.
cdmikechen commented on a change in pull request #1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path from an empty string
URL: https://github.com/apache/incubator-hudi/pull/1126#discussion_r361095095
##########
File path: hudi-client/src/main/java/org/apache/hudi/table/HoodieCopyOnWriteTable.java
##########
@@ -109,7 +109,7 @@ public HoodieCopyOnWriteTable(HoodieWriteConfig config, JavaSparkContext jsc) {
Tuple2<String, String> partitionDelFileTuple = iter.next();
String partitionPath = partitionDelFileTuple._1();
String delFileName = partitionDelFileTuple._2();
- Path deletePath = new Path(new Path(basePath, partitionPath), delFileName);
+ Path deletePath = FSUtils.getPartitionPath(FSUtils.getPartitionPath(basePath, partitionPath), delFileName);
Review comment:
@vinothchandar
If by name, it should be better to just modify`FSUtils.getPartitionPath(basePath, partitionPath)`. Maybe I should revert and put another PR?
In hadoop 2.7+, I found that `new Path()` api use more stringent checks than ever before, if user use null to use `new Path(path, null)` in hadoop2.7-, it doesn't report error.
If it's for API compatibility, is it better to adjust its name to `buildMultiPath()` or else?So that all similar methods can be used, thus to ensure that no exception will happen.
----------------------------------------------------------------
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 #1126: Fix Error: java.lang.IllegalArgumentException: Can not
create a Path from an empty string
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path from an empty string
URL: https://github.com/apache/incubator-hudi/pull/1126#discussion_r361079699
##########
File path: hudi-client/src/main/java/org/apache/hudi/table/HoodieCopyOnWriteTable.java
##########
@@ -109,7 +109,7 @@ public HoodieCopyOnWriteTable(HoodieWriteConfig config, JavaSparkContext jsc) {
Tuple2<String, String> partitionDelFileTuple = iter.next();
String partitionPath = partitionDelFileTuple._1();
String delFileName = partitionDelFileTuple._2();
- Path deletePath = new Path(new Path(basePath, partitionPath), delFileName);
+ Path deletePath = FSUtils.getPartitionPath(FSUtils.getPartitionPath(basePath, partitionPath), delFileName);
Review comment:
Does this simply need a `new Path(FSUtils.getPartitionPath(basePath, partitionPath), delFileName)` ? We are overloading calling of `FSUtils.getPartitionPath`. for somethings that's not a partitionpath?
Also can we please ensure there is a JIRA for fix.. or we follow the `[MINOR] ...` convention?
@cdmikechen @yanghua 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
With regards,
Apache Git Services
[GitHub] [incubator-hudi] yanghua commented on a change in pull request
#1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path
from an empty string
Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path from an empty string
URL: https://github.com/apache/incubator-hudi/pull/1126#discussion_r361082265
##########
File path: hudi-client/src/main/java/org/apache/hudi/table/HoodieCopyOnWriteTable.java
##########
@@ -109,7 +109,7 @@ public HoodieCopyOnWriteTable(HoodieWriteConfig config, JavaSparkContext jsc) {
Tuple2<String, String> partitionDelFileTuple = iter.next();
String partitionPath = partitionDelFileTuple._1();
String delFileName = partitionDelFileTuple._2();
- Path deletePath = new Path(new Path(basePath, partitionPath), delFileName);
+ Path deletePath = FSUtils.getPartitionPath(FSUtils.getPartitionPath(basePath, partitionPath), delFileName);
Review comment:
> Does this simply need a `new Path(FSUtils.getPartitionPath(basePath, partitionPath), delFileName)` ? We are overloading calling of `FSUtils.getPartitionPath`. for somethings that's not a partitionpath?
I saw `FsUtils#getPartitionPath` can only avoid the second arg to be null. However, the outer `new Path(..., delFileName);` The `delFileName` can not be null?
> Also can we please ensure there is a JIRA for fix.. or we follow the `[MINOR] ...` convention?
>
Agree, I have mentioned in the approvement comment. Not sure whether `[MINOR]` is a normal prefix if there is not a Jira issue to track. IMO, We should clearly record where. Or does it exist but I don't know?
----------------------------------------------------------------
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 #1126: Fix Error: java.lang.IllegalArgumentException: Can not
create a Path from an empty string
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path from an empty string
URL: https://github.com/apache/incubator-hudi/pull/1126#discussion_r362297795
##########
File path: hudi-client/src/main/java/org/apache/hudi/table/HoodieCopyOnWriteTable.java
##########
@@ -109,7 +109,7 @@ public HoodieCopyOnWriteTable(HoodieWriteConfig config, JavaSparkContext jsc) {
Tuple2<String, String> partitionDelFileTuple = iter.next();
String partitionPath = partitionDelFileTuple._1();
String delFileName = partitionDelFileTuple._2();
- Path deletePath = new Path(new Path(basePath, partitionPath), delFileName);
+ Path deletePath = FSUtils.getPartitionPath(FSUtils.getPartitionPath(basePath, partitionPath), delFileName);
Review comment:
I think we can do a follow up small fix here.. confirm `delFileName` cannot be null and then remove the outer `FSUtils.getPartitionPath`. This one can have `[MINOR]` prefix IMO
----------------------------------------------------------------
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 #1126: Fix Error:
java.lang.IllegalArgumentException: Can not create a Path from an empty
string
Posted by GitBox <gi...@apache.org>.
yanghua merged pull request #1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path from an empty string
URL: https://github.com/apache/incubator-hudi/pull/1126
----------------------------------------------------------------
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] cdmikechen commented on a change in pull request
#1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path
from an empty string
Posted by GitBox <gi...@apache.org>.
cdmikechen commented on a change in pull request #1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path from an empty string
URL: https://github.com/apache/incubator-hudi/pull/1126#discussion_r361095141
##########
File path: hudi-client/src/main/java/org/apache/hudi/table/HoodieCopyOnWriteTable.java
##########
@@ -109,7 +109,7 @@ public HoodieCopyOnWriteTable(HoodieWriteConfig config, JavaSparkContext jsc) {
Tuple2<String, String> partitionDelFileTuple = iter.next();
String partitionPath = partitionDelFileTuple._1();
String delFileName = partitionDelFileTuple._2();
- Path deletePath = new Path(new Path(basePath, partitionPath), delFileName);
+ Path deletePath = FSUtils.getPartitionPath(FSUtils.getPartitionPath(basePath, partitionPath), delFileName);
Review comment:
@vinothchandar
Yeah, I will open a JIRA issue first next time and then decide whether to submit PR according to the question.
If by name, it should be better to just modify`FSUtils.getPartitionPath(basePath, partitionPath)`. Maybe I should revert and put another PR?
In hadoop 2.7+, I found that `new Path()` api use more stringent checks than ever before, if user use null to use `new Path(path, null)` in hadoop2.7-, it doesn't report error.
If it's for API compatibility, is it better to adjust its name to `buildMultiPath()` or else?So that all similar methods can be used, thus to ensure that no exception will happen.
----------------------------------------------------------------
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] cdmikechen commented on a change in pull request
#1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path
from an empty string
Posted by GitBox <gi...@apache.org>.
cdmikechen commented on a change in pull request #1126: Fix Error: java.lang.IllegalArgumentException: Can not create a Path from an empty string
URL: https://github.com/apache/incubator-hudi/pull/1126#discussion_r361095141
##########
File path: hudi-client/src/main/java/org/apache/hudi/table/HoodieCopyOnWriteTable.java
##########
@@ -109,7 +109,7 @@ public HoodieCopyOnWriteTable(HoodieWriteConfig config, JavaSparkContext jsc) {
Tuple2<String, String> partitionDelFileTuple = iter.next();
String partitionPath = partitionDelFileTuple._1();
String delFileName = partitionDelFileTuple._2();
- Path deletePath = new Path(new Path(basePath, partitionPath), delFileName);
+ Path deletePath = FSUtils.getPartitionPath(FSUtils.getPartitionPath(basePath, partitionPath), delFileName);
Review comment:
@vinothchandar
If by name, it should be better to just modify`FSUtils.getPartitionPath(basePath, partitionPath)`. Maybe I should revert and put another PR?
In hadoop 2.7+, I found that `new Path()` api use more stringent checks than ever before, if user use null to use `new Path(path, null)` in hadoop2.7-, it doesn't report error.
If it's for API compatibility, is it better to adjust its name to `buildMultiPath()` or else?So that all similar methods can be used, thus to ensure that no exception will happen.
----------------------------------------------------------------
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