You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@carbondata.apache.org by GitBox <gi...@apache.org> on 2021/03/12 07:31:51 UTC
[GitHub] [carbondata] jack86596 opened a new pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
jack86596 opened a new pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105
### Why is this PR needed?
Reindex failed when SI has stale carbonindexmerge file, throw exception FileNotFoundException. This is because SegmentFileStore.getIndexFiles stores the mapping of indexfile to indexmergefile, when stale carbon indexmergefile exists, indexmergefile will not be null. During merging index file, new indexmergefile will be created with same name as before in the same location. At the end of CarbonIndexFileMergeWriter.writeMergeIndexFileBasedOnSegmentFile, carbon index file will be deleted. Since indexmergefile is stored in the indexFiles list, newly created indexmergefile will be delete also, which leads to FileNotFoundException.
### What changes were proposed in this PR?
1. SegmentFileStore.getIndexFiles stores the mapping of indexfile to indexmergefile which is redundant.
2. SegmentFileStore.getIndexOrMergeFiles returns both index file and index merge file, so function name is incorrect, rename to getIndexAndMergeFiles.
3. CarbonLoaderUtil.getActiveExecutor actually get active node, so function name is incorrect, rename to getActiveNode, together replace all "executor" with "node" in function assignBlocksByDataLocality.
### Does this PR introduce any user interface change?
- No
### Is any new testcase added?
- 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
[GitHub] [carbondata] jack86596 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-801552836
retest this please
----------------------------------------------------------------
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] [carbondata] Indhumathi27 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
Indhumathi27 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595146990
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
1. User can face this scenario, what i meant is, allowing delete directly on index Table is also a new issue, when user does it by mistake, will make main table and SI not in sync. Then, user unneesarily has to perform Reindex. So, you can handle this scenario in this PR.
2. If you are gonna return only index Files, always merge file name will be null. So, you can return List of index Files alone instead of Map from getIndexFiles 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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595136721
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
1. "i think, at first, delete segment directly from indexTable should not be allowed." This is unit test, we are simulating the issue(main table semgent success and SI table segment MFD), for real user, they don't need to manually delete SI segment in order to get the issue, they will face this issue directly in their daily running without any manuallly operation.
2. "For second case, where main table contains SUCCESS segments and SI has MFD, during reindex, before loading data again, we can delete old stale files in that segment" Yes, you provide a solution and i agree this will work, but again, the solution you provide is not as good as current one because current one you don't need to delete the stale files, reindex can also work.
----------------------------------------------------------------
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] [carbondata] Indhumathi27 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
Indhumathi27 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-802560990
retest this please
--
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-797371226
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3796/
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-801600161
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3813/
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-800567860
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3805/
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-801600676
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5579/
----------------------------------------------------------------
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] [carbondata] Indhumathi27 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
Indhumathi27 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595180273
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
ok
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595193924
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -854,7 +853,7 @@ public SegmentFile getSegmentFile() {
* Gets all index files from this segment
* @return
*/
- public Map<String, String> getIndexOrMergeFiles() throws IOException {
+ public Map<String, String> getIndexAndMergeFiles() throws IOException {
Review comment:
Or means either, not both, but sometime it will return both index file and merge file, so Or is not correct.
----------------------------------------------------------------
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] [carbondata] VenuReddy2103 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
VenuReddy2103 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595165241
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -841,8 +841,7 @@ public SegmentFile getSegmentFile() {
}
if (entry.getValue().status.equals(SegmentStatus.SUCCESS.getMessage())) {
for (String indexFile : entry.getValue().getFiles()) {
- indexFiles.put(location + CarbonCommonConstants.FILE_SEPARATOR + indexFile,
- entry.getValue().mergeFileName);
+ indexFiles.put(location + CarbonCommonConstants.FILE_SEPARATOR + indexFile, null);
Review comment:
Its caller `SegmentFileStore.getIndexCarbonFiles()`has code to handle for non null value to add merge index file also to index file list(at line 906). It becomes dead code now. You would want to remove that too.
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595167863
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
1. For example, if both main table and SI table segment status are success, but one of the carbondata file or index file in SI segment is missing or broken, how you fix this if you cannot manually delete this SI segment?
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r594143885
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
So please provide a better solution to solve this issue: main table segment (success), SI table segment (marked for delete). The solution should not be "run clean files for SI table first", because it is not better than current one.
----------------------------------------------------------------
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] [carbondata] Indhumathi27 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
Indhumathi27 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595146990
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
1. User can face this scenario, what i meant is, allowing delete directly on index Table is also a new issue, when user does it by mistake, will make main table and SI not in sync. Then, user unneesarily has to perform Reindex. So, you can handle this scenario in this PR.
2. If you are gonna return only index Files, always merge file name will be null. So, you can use return List of index Files alone instead of Map from getIndexFiles 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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595205788
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -854,7 +853,7 @@ public SegmentFile getSegmentFile() {
* Gets all index files from this segment
* @return
*/
- public Map<String, String> getIndexOrMergeFiles() throws IOException {
+ public Map<String, String> getIndexAndMergeFiles() throws IOException {
Review comment:
Current implementation:
if (null != mergeFileName) {
add merge file
}
if (null != index file && not empty) {
add index file
}
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-802722165
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3835/
--
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-801406211
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5576/
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595193924
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -854,7 +853,7 @@ public SegmentFile getSegmentFile() {
* Gets all index files from this segment
* @return
*/
- public Map<String, String> getIndexOrMergeFiles() throws IOException {
+ public Map<String, String> getIndexAndMergeFiles() throws IOException {
Review comment:
Or means either, not both, but the implementation shows it will return both index file and merge file, so Or is not correct.
----------------------------------------------------------------
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] [carbondata] VenuReddy2103 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
VenuReddy2103 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595256317
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
Agree with @Indhumathi27. IMHO, we shouldn't allow delete segments on index table itself. And during repair index, if have segment with partial data, we should delete the segment completely(segment folder, segment file, probably tablestatus entry for the segment as well) before proceeding with segment repair.
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-802619404
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5597/
--
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] [carbondata] Indhumathi27 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
Indhumathi27 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595110340
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
i think, at first, delete segment directly from indexTable should not be allowed. For second case, where main table contains SUCCESS segments and SI has MFD, during reindex, before loading data again, we can delete old stale files in that segment
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595408209
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -841,8 +841,7 @@ public SegmentFile getSegmentFile() {
}
if (entry.getValue().status.equals(SegmentStatus.SUCCESS.getMessage())) {
for (String indexFile : entry.getValue().getFiles()) {
- indexFiles.put(location + CarbonCommonConstants.FILE_SEPARATOR + indexFile,
- entry.getValue().mergeFileName);
+ indexFiles.put(location + CarbonCommonConstants.FILE_SEPARATOR + indexFile, null);
Review comment:
Done.
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595161142
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
1. User will not delete SI segment directly, the issue (main table segment success and SI segment MFD) is because carbon has so many bugs which will lead to this situation even user only just do the loading and compaction. And actually in production, we (maintainers) need to "delete directly on index table" just because we need to recovery carbon table from broken state to normal state, then later trigger reindex to repair the broken SI segment. So until "all the bugs which will lead to main table and SI table not in syc" are solved, we cannot block this.
2. This "return List of index Files alone" if you say so, I can change it.
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-800566842
Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5571/
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595406480
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
1. "we shouldn't allow delete segments on index table itself." please refer to the second last comment right before your comment.
2. "And during repair index, if have segment with partial data, we should delete the segment completely(segment folder, segment file, probably tablestatus entry for the segment as well) before proceeding with segment repair." You suggestion of course is right but too more complicate comparing to existing implementation, so please first do the complete analysis and design and then we can discuss and plan next step.
----------------------------------------------------------------
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] [carbondata] jack86596 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-800730205
retest this please
----------------------------------------------------------------
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] [carbondata] jack86596 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-799227509
> @jack86596 , won't running clean files before doing the reindex operation solve this issue?
why before reindex operation, need to run clean files? and dependency relation between reindex and clean files?
----------------------------------------------------------------
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] [carbondata] Indhumathi27 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
Indhumathi27 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595110340
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
i think, at first, delete segment directly from indexTable should not be allowed. For second case, where main table contains SUCCESS segments and SI has MFD, during reindex, before loading data again, we can delete old files in that segment
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595406480
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
1. "we shouldn't allow delete segments on index table itself." please refer to the second last comment right before your comment. If you ever solve production issue, you could not say this. There are thousand of query failed issues just because of SI segment is broken. We need to first delete the broken SI segment then repair it again(last two to three years, countless issues because of SI segment broken or not sync with main table). So please get to know customer, not build software without any knowing about how customer use the software. And please during coding, stand also at maintainer side, implement the feature with more maintainability. Thanks.
2. "And during repair index, if have segment with partial data, we should delete the segment completely(segment folder, segment file, probably tablestatus entry for the segment as well) before proceeding with segment repair." You suggestion of course is right but too more complicate comparing to existing implementation, so please first do the complete analysis and design and then we can discuss and plan next step.
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595175202
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -841,8 +841,7 @@ public SegmentFile getSegmentFile() {
}
if (entry.getValue().status.equals(SegmentStatus.SUCCESS.getMessage())) {
for (String indexFile : entry.getValue().getFiles()) {
- indexFiles.put(location + CarbonCommonConstants.FILE_SEPARATOR + indexFile,
- entry.getValue().mergeFileName);
+ indexFiles.put(location + CarbonCommonConstants.FILE_SEPARATOR + indexFile, null);
Review comment:
This make sense, will do it.
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-802619029
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3831/
--
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] [carbondata] vikramahuja1001 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
vikramahuja1001 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r594123727
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
technically speaking reindex should not be allowed at this point, because the segment still exists(even if it is MFD), the purpose of reindex command is to reload missing segments which is not the case here. Also, will this same behaviour also happen for other segment status, say success or partially success? @akashrn5 can give some input maybe
----------------------------------------------------------------
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] [carbondata] Indhumathi27 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
Indhumathi27 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595180273
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
ok. keep current behavior
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-802712514
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5601/
--
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] [carbondata] VenuReddy2103 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
VenuReddy2103 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595165241
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -841,8 +841,7 @@ public SegmentFile getSegmentFile() {
}
if (entry.getValue().status.equals(SegmentStatus.SUCCESS.getMessage())) {
for (String indexFile : entry.getValue().getFiles()) {
- indexFiles.put(location + CarbonCommonConstants.FILE_SEPARATOR + indexFile,
- entry.getValue().mergeFileName);
+ indexFiles.put(location + CarbonCommonConstants.FILE_SEPARATOR + indexFile, null);
Review comment:
Its caller `SegmentFileStore.getIndexCarbonFiles()`has code to handle for non null value to add merge index file also to index file list. It becomes dead code now. You would want to remove that too.
----------------------------------------------------------------
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] [carbondata] vikramahuja1001 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
vikramahuja1001 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-799208880
@jack86596 , won't running clean files before doing the reindex operation solve this issue?
----------------------------------------------------------------
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] [carbondata] jack86596 edited a comment on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 edited a comment on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-799346641
> > > @jack86596 , won't running clean files before doing the reindex operation solve this issue?
> >
> >
> > why before reindex operation, need to run clean files? any dependency relation between reindex and clean files?
>
> No, there is no need to do clean files before reindex there is no dependency, but we only do reindex when the segments are missing, it is not the case here though. What i meant was that running clean files on si table would solve your problem
why I need to run one extra command to solve the problem, why not just solve the problem directly, in this way, no need to run extra command? If in order to solve the problem, you need to run clean files, then this is dependency(The success of reindex command depends on clean files command).
----------------------------------------------------------------
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] [carbondata] jack86596 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-799346641
> > > @jack86596 , won't running clean files before doing the reindex operation solve this issue?
> >
> >
> > why before reindex operation, need to run clean files? any dependency relation between reindex and clean files?
>
> No, there is no need to do clean files before reindex there is no dependency, but we only do reindex when the segments are missing, it is not the case here though. What i meant was that running clean files on si table would solve your problem
why I need to run one extra command to solve the problem, why not just solve the problem directly, in this way, no need to run extra command? If in order to solve the problem, you need to run clean files, then this is dependency.
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595181102
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -854,7 +853,7 @@ public SegmentFile getSegmentFile() {
* Gets all index files from this segment
* @return
*/
- public Map<String, String> getIndexOrMergeFiles() throws IOException {
+ public Map<String, String> getIndexAndMergeFiles() throws IOException {
Review comment:
backward compatibility, carbon 1.3 both index and merge file exist.
----------------------------------------------------------------
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] [carbondata] asfgit closed pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105
--
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] [carbondata] jack86596 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-802629344
retest this please
--
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-797370576
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5562/
----------------------------------------------------------------
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] [carbondata] Indhumathi27 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
Indhumathi27 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-801628181
LGTM
----------------------------------------------------------------
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] [carbondata] jack86596 edited a comment on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 edited a comment on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-799227509
> @jack86596 , won't running clean files before doing the reindex operation solve this issue?
why before reindex operation, need to run clean files? any dependency relation between reindex and clean files?
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-800773654
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3806/
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-801419839
Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3810/
----------------------------------------------------------------
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] [carbondata] VenuReddy2103 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
VenuReddy2103 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595178820
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -854,7 +853,7 @@ public SegmentFile getSegmentFile() {
* Gets all index files from this segment
* @return
*/
- public Map<String, String> getIndexOrMergeFiles() throws IOException {
+ public Map<String, String> getIndexAndMergeFiles() throws IOException {
Review comment:
I still think getIndexOrMergeFiles name makes sense. Returned map can have either mergeindex or index. Curious to know when it does have both merge index and index in map ?
----------------------------------------------------------------
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] [carbondata] CarbonDataQA2 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
CarbonDataQA2 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-800772914
Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5572/
----------------------------------------------------------------
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] [carbondata] jack86596 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
jack86596 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r596494501
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -898,15 +897,8 @@ public SegmentFile getSegmentFile() {
* @return
*/
public List<CarbonFile> getIndexCarbonFiles() {
- Map<String, String> indexFiles = getIndexFiles();
- Set<String> files = new HashSet<>();
- for (Map.Entry<String, String> entry: indexFiles.entrySet()) {
- Path path = new Path(entry.getKey());
- files.add(entry.getKey());
- if (entry.getValue() != null) {
- files.add(new Path(path.getParent(), entry.getValue()).toString());
- }
- }
+ List<String> indexFiles = getIndexFiles();
Review comment:
done
----------------------------------------------------------------
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] [carbondata] Indhumathi27 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
Indhumathi27 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595857079
##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/SegmentFileStore.java
##########
@@ -898,15 +897,8 @@ public SegmentFile getSegmentFile() {
* @return
*/
public List<CarbonFile> getIndexCarbonFiles() {
- Map<String, String> indexFiles = getIndexFiles();
- Set<String> files = new HashSet<>();
- for (Map.Entry<String, String> entry: indexFiles.entrySet()) {
- Path path = new Path(entry.getKey());
- files.add(entry.getKey());
- if (entry.getValue() != null) {
- files.add(new Path(path.getParent(), entry.getValue()).toString());
- }
- }
+ List<String> indexFiles = getIndexFiles();
Review comment:
Can you return Set<String> from getIndexFiles itself
----------------------------------------------------------------
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] [carbondata] vikramahuja1001 commented on pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
vikramahuja1001 commented on pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#issuecomment-799320505
> > @jack86596 , won't running clean files before doing the reindex operation solve this issue?
>
> why before reindex operation, need to run clean files? any dependency relation between reindex and clean files?
No, there is no need to do clean files before reindex there is no dependency, but we only do reindex when the segments are missing, it is not the case here though. What i meant was that running clean files on si table would solve your problem
----------------------------------------------------------------
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] [carbondata] VenuReddy2103 commented on a change in pull request #4105: [CARBONDATA-4148] Reindex failed when SI has stale carbonindexmerge file
Posted by GitBox <gi...@apache.org>.
VenuReddy2103 commented on a change in pull request #4105:
URL: https://github.com/apache/carbondata/pull/4105#discussion_r595256317
##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestIndexRepair.scala
##########
@@ -119,6 +119,19 @@ class TestIndexRepair extends QueryTest with BeforeAndAfterAll {
sql("drop table if exists maintable")
}
+ test("reindex command with stale files") {
+ sql("drop table if exists maintable")
+ sql("CREATE TABLE maintable(a INT, b STRING, c STRING) stored as carbondata")
+ sql("CREATE INDEX indextable1 on table maintable(c) as 'carbondata'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("INSERT INTO maintable SELECT 1,'string1', 'string2'")
+ sql("DELETE FROM TABLE INDEXTABLE1 WHERE SEGMENT.ID IN(0,1,2)")
+ sql("REINDEX INDEX TABLE indextable1 ON MAINTABLE WHERE SEGMENT.ID IN (0,1)")
Review comment:
Agree with @Indhumathi27. IMHO, we shouldn't allow delete segments on index table itself. And during repair index, if have segment with partial data, we should delete the segment completely(segment folder, segment file, tablestatus entry for the segment) before proceeding with segment repair.
----------------------------------------------------------------
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