Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CARBONDATA-4146]Query fails and the error message "unable to get file status" is displayed. query is normal after the "drop metacache on table" command is executed. #4104

Closed
wants to merge 2 commits into from

Conversation

liuhe0702
Copy link
Contributor

Why is this PR needed?
During compact execution, the status of the new segment is set to success before index files are merged.
After index files are merged, the carbonindex files are deleted.
As a result, the query task cannot find the cached carbonindex files.

What changes were proposed in this PR?
Set the status of the new segment to succeeded after index files are merged.

Does this PR introduce any user interface change?
No

Is any new testcase added?
No

@CarbonDataQA2
Copy link

Can one of the admins verify this patch?

@brijoobopanna
Copy link
Contributor

add to whitelist

@CarbonDataQA2
Copy link

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3780/

@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5545/

…e status" is displayed. query is normal after the "drop metacache on table" command is executed.

Why is this PR needed?
During compact execution, the status of the new segment is set to success before index files are merged.
After index files are merged, the carbonindex files are deleted.
As a result, the query task cannot find the cached carbonindex files.

What changes were proposed in this PR?
Set the status of the new segment to succeeded after index files are merged.

Does this PR introduce any user interface change?
No

Is any new testcase added?
No
@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5554/

@CarbonDataQA2
Copy link

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3788/

@liuhe0702
Copy link
Contributor Author

retest this please

@CarbonDataQA2
Copy link

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3792/

@CarbonDataQA2
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5558/

@liuhe0702
Copy link
Contributor Author

retest this please

@CarbonDataQA2
Copy link

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3794/

@CarbonDataQA2
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5560/

@liuhe0702
Copy link
Contributor Author

retest this please

@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5561/

@CarbonDataQA2
Copy link

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3795/

@liuhe0702
Copy link
Contributor Author

retest this please

@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5563/

@CarbonDataQA2
Copy link

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3797/

@liuhe0702
Copy link
Contributor Author

retest this please

@CarbonDataQA2
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5565/

@CarbonDataQA2
Copy link

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3799/

@akashrn5
Copy link
Contributor

akashrn5 commented Mar 16, 2021

@liuhe0702 i think this issue is already being handled in #3988 , so i think no need of this PR, please refer the jira to the jira of #3988 and close as duplicate. @ShreelekhyaG please confirm the same here, where this scenario is being handled or not in your PR.

@ShreelekhyaG
Copy link
Contributor

Yes @akashrn5/ @liuhe0702 , the same scenario being handled in PR #3988

@akashrn5
Copy link
Contributor

@liuhe0702 please make the jira as duplicate and comment there as its handled in #3988 and close both jira and PR, thanks

@liuhe0702 liuhe0702 changed the title [CARBONDATA-4146]Query fails and the error message "unable to get file status" is displayed. query is normal after the "drop metacache on table" command is executed. [CARBONDATA-4146]During compaction, the segment lock of SI table is not released in abnormal scenarios. Mar 17, 2021
@liuhe0702 liuhe0702 changed the title [CARBONDATA-4146]During compaction, the segment lock of SI table is not released in abnormal scenarios. [CARBONDATA-4146]Query fails and the error message "unable to get file status" is displayed. query is normal after the "drop metacache on table" command is executed. Mar 17, 2021
@liuhe0702 liuhe0702 closed this Mar 17, 2021
@liuhe0702 liuhe0702 reopened this Mar 18, 2021
@@ -190,7 +200,7 @@ public boolean execute(List<RawResultIterator> unsortedResultIteratorList,
LOGGER.error(e.getLocalizedMessage(), e);
throw e;
} finally {
if (partitionSpec != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing boolean value, cant we directly get CarbonCommonConstants.CARBON_MERGE_INDEX_IN_SEGMENT from properties? and the if check should be - if merge index not enabled, then write segment file. As below,

 ```  boolean isMergeIndexEnable = Boolean.parseBoolean(CarbonProperties.getInstance().getProperty(
        CarbonCommonConstants.CARBON_MERGE_INDEX_IN_SEGMENT,
        CarbonCommonConstants.CARBON_MERGE_INDEX_IN_SEGMENT_DEFAULT));
    if (partitionSpec != null && !isMergeIndexEnable) {  ```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, @liuhe0702 instead of making parameter changes or adding constructors, check property here only and can avoid changes in MergerRDD

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@liuhe0702 liuhe0702 force-pushed the MergeIndexFilesFirst branch 3 times, most recently from 5c197b4 to 937f644 Compare March 19, 2021 08:46
@CarbonDataQA2
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5603/

@CarbonDataQA2
Copy link

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3837/

@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/5059/

@CarbonDataQA2
Copy link

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3307/

@liuhe0702
Copy link
Contributor Author

retest this please

@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5069/

@CarbonDataQA2
Copy link

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3316/

@liuhe0702
Copy link
Contributor Author

retest this please

@CarbonDataQA2
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5072/

CarbonCommonConstants.CARBON_MERGE_INDEX_IN_SEGMENT_DEFAULT));
// mergeIndex is true, the segment file not need to be wrotten
// and will be wrotten during merging index
if (partitionSpec != null && !isMergeIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrotten to written

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@CarbonDataQA2
Copy link

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3319/

…e status" is displayed. query is normal after the "drop metacache on table" command is executed.

not write temp segment file when mergeindex is true
@CarbonDataQA2
Copy link

Build Failed with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5074/

@CarbonDataQA2
Copy link

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3321/

@liuhe0702
Copy link
Contributor Author

retest this please

@CarbonDataQA2
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5076/

@CarbonDataQA2
Copy link

Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3323/

@liuhe0702
Copy link
Contributor Author

retest this please

@CarbonDataQA2
Copy link

Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5077/

@CarbonDataQA2
Copy link

Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3324/

@ShreelekhyaG
Copy link
Contributor

LGTM

1 similar comment
@akashrn5
Copy link
Contributor

LGTM

@asfgit asfgit closed this in 6ab3647 Mar 23, 2021
Indhumathi27 pushed a commit to Indhumathi27/carbondata that referenced this pull request Apr 22, 2021
…e status" is displayed.

query is normal after the "drop metacache on table" command is executed.

Why is this PR needed?
During compact execution, the status of the new segment is set to success before index
files are merged. After index files are merged, the carbonindex files are deleted.
As a result, the query task cannot find the cached carbonindex files.

What changes were proposed in this PR?
Set the status of the new segment to succeeded after index files are merged.

Does this PR introduce any user interface change?
No

Is any new testcase added?
No

This closes apache#4104
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants