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/07/20 22:59:53 UTC
[GitHub] [hudi] satishkotha opened a new pull request #1853: [HUDI-1072] Add replace metadata file to timeline
satishkotha opened a new pull request #1853:
URL: https://github.com/apache/hudi/pull/1853
## What is the purpose of the pull request
This is part of work required for RFC-18 and RFC-19. Add replace action to valid actions in the timeline.
To keep the diff small and get feedback, i am sending just the structure of metadata. For examples of how this will be used, see POC here https://github.com/satishkotha/incubator-hudi/commit/45d8c26b407b2b5329925cb0ab5af93e293a1cae.
I'm happy to bring in other changes from POC if you think we can push large change.
## Brief change log
* Add replace action to valid actions in the timeline
* Add replace metadata file format
## Verify this pull request
This change added unit 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
[GitHub] [hudi] vinothchandar commented on pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#issuecomment-683874575
@satishkotha is this WIP?
----------------------------------------------------------------
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] n3nash commented on a change in pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
n3nash commented on a change in pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#discussion_r458475548
##########
File path: hudi-common/src/main/avro/HoodieReplaceMetadata.avsc
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+ /*
+ * Note that all 'replace' instants are read for every query
+ * So it is important to keep this small. Please be careful
+ * before tracking additional information in this file.
+ * This will be used for 'insert_overwrite' (RFC-18) and also 'clustering' (RFC-19)
+ */
+{"namespace": "org.apache.hudi.avro.model",
+ "type": "record",
+ "name": "HoodieReplaceMetadata",
+ "fields": [
+ {"name": "totalFilesReplaced", "type": "int"},
+ {"name": "command", "type": "string"},
+ {"name": "partitionMetadata", "type": {
Review comment:
Does the partitionMetadata contain partitionPath -> new file groups ?
----------------------------------------------------------------
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] satishkotha commented on a change in pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
satishkotha commented on a change in pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#discussion_r458510585
##########
File path: hudi-common/src/main/avro/HoodieReplaceMetadata.avsc
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+ /*
+ * Note that all 'replace' instants are read for every query
+ * So it is important to keep this small. Please be careful
+ * before tracking additional information in this file.
+ * This will be used for 'insert_overwrite' (RFC-18) and also 'clustering' (RFC-19)
+ */
+{"namespace": "org.apache.hudi.avro.model",
+ "type": "record",
+ "name": "HoodieReplaceMetadata",
+ "fields": [
+ {"name": "totalFilesReplaced", "type": "int"},
+ {"name": "command", "type": "string"},
+ {"name": "partitionMetadata", "type": {
Review comment:
This can be obtained from related commit file. Every t.replace has a corresponding t.[delta]commit (we decided to write two files to reduce overhead of reading all commit files in the query path)
Plus, it helps on query side to keep replace metadata files small. I also didn't want to repeat same information in two places.
Let me know if you think its important to store new file group info here.
##########
File path: hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java
##########
@@ -304,6 +305,22 @@ public HoodieInstant transitionCleanRequestedToInflight(HoodieInstant requestedI
return inflight;
}
+ /**
+ * Transition Clean State from inflight to Committed.
Review comment:
will update. thanks
----------------------------------------------------------------
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] n3nash commented on a change in pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
n3nash commented on a change in pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#discussion_r458476085
##########
File path: hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java
##########
@@ -304,6 +305,22 @@ public HoodieInstant transitionCleanRequestedToInflight(HoodieInstant requestedI
return inflight;
}
+ /**
+ * Transition Clean State from inflight to Committed.
Review comment:
s/Clean/Replace
----------------------------------------------------------------
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] satishkotha commented on pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
satishkotha commented on pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#issuecomment-684105435
This has been replaced by #2048
----------------------------------------------------------------
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] n3nash commented on a change in pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
n3nash commented on a change in pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#discussion_r458476453
##########
File path: hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java
##########
@@ -65,7 +65,8 @@
COMMIT_EXTENSION, INFLIGHT_COMMIT_EXTENSION, REQUESTED_COMMIT_EXTENSION, DELTA_COMMIT_EXTENSION,
INFLIGHT_DELTA_COMMIT_EXTENSION, REQUESTED_DELTA_COMMIT_EXTENSION, SAVEPOINT_EXTENSION,
INFLIGHT_SAVEPOINT_EXTENSION, CLEAN_EXTENSION, REQUESTED_CLEAN_EXTENSION, INFLIGHT_CLEAN_EXTENSION,
- INFLIGHT_COMPACTION_EXTENSION, REQUESTED_COMPACTION_EXTENSION, INFLIGHT_RESTORE_EXTENSION, RESTORE_EXTENSION));
+ INFLIGHT_COMPACTION_EXTENSION, REQUESTED_COMPACTION_EXTENSION, INFLIGHT_RESTORE_EXTENSION, RESTORE_EXTENSION,
Review comment:
Since the intention of this PR is to simply introduce a new action, do we need to change the VALID_ACTION in this PR ? Do the other methods you implemented in the ActiveTimeline require this ? I'm wondering if we can avoid this change in case we want to land this PR incrementally
----------------------------------------------------------------
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] satishkotha commented on a change in pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
satishkotha commented on a change in pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#discussion_r458511328
##########
File path: hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieTimeline.java
##########
@@ -126,6 +129,13 @@
*/
HoodieTimeline getCommitsAndCompactionTimeline();
+ /**
+ * Timeline to just include replace instants that have valid (commit/deltacommit) actions.
+ *
+ * @return
+ */
+ HoodieTimeline getCompletedAndReplaceTimeline();
Review comment:
yes, this is used to build filesystem view in #1859, can move this to that PR
----------------------------------------------------------------
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] satishkotha commented on a change in pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
satishkotha commented on a change in pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#discussion_r458510963
##########
File path: hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java
##########
@@ -65,7 +65,8 @@
COMMIT_EXTENSION, INFLIGHT_COMMIT_EXTENSION, REQUESTED_COMMIT_EXTENSION, DELTA_COMMIT_EXTENSION,
INFLIGHT_DELTA_COMMIT_EXTENSION, REQUESTED_DELTA_COMMIT_EXTENSION, SAVEPOINT_EXTENSION,
INFLIGHT_SAVEPOINT_EXTENSION, CLEAN_EXTENSION, REQUESTED_CLEAN_EXTENSION, INFLIGHT_CLEAN_EXTENSION,
- INFLIGHT_COMPACTION_EXTENSION, REQUESTED_COMPACTION_EXTENSION, INFLIGHT_RESTORE_EXTENSION, RESTORE_EXTENSION));
+ INFLIGHT_COMPACTION_EXTENSION, REQUESTED_COMPACTION_EXTENSION, INFLIGHT_RESTORE_EXTENSION, RESTORE_EXTENSION,
Review comment:
I added this for tests. But can split this and related test out into a different review
----------------------------------------------------------------
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] bvaradar commented on a change in pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
bvaradar commented on a change in pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#discussion_r461124646
##########
File path: hudi-common/src/main/avro/HoodieReplaceMetadata.avsc
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+ /*
+ * Note that all 'replace' instants are read for every query
+ * So it is important to keep this small. Please be careful
+ * before tracking additional information in this file.
+ * This will be used for 'insert_overwrite' (RFC-18) and also 'clustering' (RFC-19)
+ */
+{"namespace": "org.apache.hudi.avro.model",
+ "type": "record",
+ "name": "HoodieReplaceMetadata",
+ "fields": [
+ {"name": "totalFilesReplaced", "type": "int"},
+ {"name": "command", "type": "string"},
+ {"name": "partitionMetadata", "type": {
Review comment:
High level Question : To make sure we are all on the same page : Is this metadata enough to achieve clustering ? Do you foresee any changes that needs to happen to this metadata to support clustering ? The PR mentions that this is for both clustering and overwrite. Hence, asking this question.
##########
File path: hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieTimeline.java
##########
@@ -126,6 +129,13 @@
*/
HoodieTimeline getCommitsAndCompactionTimeline();
+ /**
+ * Timeline to just include replace instants that have valid (commit/deltacommit) actions.
+ *
+ * @return
+ */
+ HoodieTimeline getCompletedAndReplaceTimeline();
Review comment:
Does the return timeline contains only replace timeline ? The naming is confusing to me. How about getValidReplaceTimeline or anything else to reflect the intention
##########
File path: hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieTimeline.java
##########
@@ -57,7 +58,7 @@
String[] VALID_ACTIONS_IN_TIMELINE = {COMMIT_ACTION, DELTA_COMMIT_ACTION,
CLEAN_ACTION, SAVEPOINT_ACTION, RESTORE_ACTION, ROLLBACK_ACTION,
- COMPACTION_ACTION};
+ COMPACTION_ACTION, REPLACE_ACTION};
Review comment:
same comment as above
##########
File path: hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java
##########
@@ -65,7 +65,8 @@
COMMIT_EXTENSION, INFLIGHT_COMMIT_EXTENSION, REQUESTED_COMMIT_EXTENSION, DELTA_COMMIT_EXTENSION,
INFLIGHT_DELTA_COMMIT_EXTENSION, REQUESTED_DELTA_COMMIT_EXTENSION, SAVEPOINT_EXTENSION,
INFLIGHT_SAVEPOINT_EXTENSION, CLEAN_EXTENSION, REQUESTED_CLEAN_EXTENSION, INFLIGHT_CLEAN_EXTENSION,
- INFLIGHT_COMPACTION_EXTENSION, REQUESTED_COMPACTION_EXTENSION, INFLIGHT_RESTORE_EXTENSION, RESTORE_EXTENSION));
+ INFLIGHT_COMPACTION_EXTENSION, REQUESTED_COMPACTION_EXTENSION, INFLIGHT_RESTORE_EXTENSION, RESTORE_EXTENSION,
Review comment:
Its better to avoid this for rollout purpose. In case, this PR gets landed before the next and a release cut, then we need to worry about ordering of rollout between readers and writers.
----------------------------------------------------------------
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] satishkotha commented on pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
satishkotha commented on pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#issuecomment-662220607
> High level, introducing `replace` action changes seem fine to me, interested in learning how old_file_group -> new_file_group mapping is stored and accessed. Yet to review the other PRs if they have this info..
This is not 1:1 mapping. So this is not being stored in replace metadata file. We can get new file groups created from corresponding commit metadata file. As far as I can see, we only need this information for auditing, debugging. So I'm planning to build this with CLI task. Let me know if you see this useful in other scenarios.
----------------------------------------------------------------
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] n3nash commented on a change in pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
n3nash commented on a change in pull request #1853:
URL: https://github.com/apache/hudi/pull/1853#discussion_r458476891
##########
File path: hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieTimeline.java
##########
@@ -126,6 +129,13 @@
*/
HoodieTimeline getCommitsAndCompactionTimeline();
+ /**
+ * Timeline to just include replace instants that have valid (commit/deltacommit) actions.
+ *
+ * @return
+ */
+ HoodieTimeline getCompletedAndReplaceTimeline();
Review comment:
May be you have used this API in a different PR, but where exactly are we going to use this specific method ?
----------------------------------------------------------------
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] satishkotha closed pull request #1853: [HUDI-1072] Add replace metadata file to timeline
Posted by GitBox <gi...@apache.org>.
satishkotha closed pull request #1853:
URL: https://github.com/apache/hudi/pull/1853
----------------------------------------------------------------
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