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.

Sign up for GitHub

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

Jump to bottom

[CARBONDATA-3957] Added property to enable disable the repair logic and provide a limit to number of segments in SILoadEventListenerForFailedSegments #3894

Closed
wants to merge 1 commit into from
Closed

[CARBONDATA-3957] Added property to enable disable the repair logic and provide a limit to number of segments in SILoadEventListenerForFailedSegments #3894

wants to merge 1 commit into from

Conversation

vikramahuja1001
Copy link
Contributor

@vikramahuja1001 vikramahuja1001 commented Aug 18, 2020 •
edited

Why is this PR needed?

In the main table with SI tables after every load/insert command , SILoadEventListenerForFailedSegments.scala checks for missing segments or segments mismatch in SI table and loads the missing/deleted segments to the SI table. In case when there are very large number of missing segments in the SI table(10000's), the repair logic will run for multiple days and will thus block the next load.

What changes were proposed in this PR?

The above mentioned issue is solved using 2 carbon properties.

  1. carbon.load.si.repair- This property lets the user enable/disable the repair SI logic in SILoadEventListenerForFailedSegments. The default value of this property is true.
  2. carbon.si.repair.limit - This property decides the number of failed segments that are being loaded again in the SI table in SILoadEventListenerForFailedSegments. By default repairing all the segments. Instead of repairing all the segments in one load the user can now set a limit thus not blocking the next load for the in case of large number of missing SI segments.

Does this PR introduce any user interface change?

  • No
  • Yes. (please explain the change and update document)

Is any new testcase added?

  • No
  • Yes

@CarbonDataQA1
Copy link

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

@CarbonDataQA1
Copy link

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

@vikramahuja1001
Copy link
Contributor Author

retest this please

@CarbonDataQA1
Copy link

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

@CarbonDataQA1
Copy link

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

@CarbonDataQA1
Copy link

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

@CarbonDataQA1
Copy link

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

@vikramahuja1001
Copy link
Contributor Author

retest this please

@vikramahuja1001
Copy link
Contributor Author

retest this please

@CarbonDataQA1
Copy link

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

@CarbonDataQA1
Copy link

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

@vikramahuja1001
Copy link
Contributor Author

@kunal642 @VenuReddy2103 @akashrn5 , please review

@vikramahuja1001 vikramahuja1001 changed the title [WIP] Added property to enable disable SIforFailed segments and added prope… [WIP] Added property to enable disable the repair logic and provide a limit to number of segments in SILoadEventListenerForFailedSegments Aug 19, 2020
@vikramahuja1001 vikramahuja1001 changed the title [WIP] Added property to enable disable the repair logic and provide a limit to number of segments in SILoadEventListenerForFailedSegments [CARBONDATA-3957] Added property to enable disable the repair logic and provide a limit to number of segments in SILoadEventListenerForFailedSegments Aug 19, 2020
kunal642
kunal642 reviewed Aug 24, 2020
View reviewed changes
core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
* SI table in the FailedSegments listener.
*/
@CarbonProperty
public static final String CARBON_SI_REPAIR_LIMIT = "carbon.si.repair.limit";
Copy link
Contributor

@kunal642 kunal642 Aug 24, 2020 •
edited

Choose a reason for hiding this comment

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

  1. I think its better to have these 2 properties at table level instead of global.

  2. If this is a dynamic conf then add (dynamicConfiguration = true) as the annotation.

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

kunal642
kunal642 reviewed Aug 24, 2020
View reviewed changes
core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java Outdated
/**
* Default value for the number of segments to be repaired during SI repair.
*/
public static final String CARBON_SI_REPAIR_LIMIT_DEFAULT = "1";
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for a default value, if the user has not set then we can go with the existing logic of repairing all the segments

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

kunal642
kunal642 reviewed Aug 24, 2020
View reviewed changes
core/src/main/java/org/apache/carbondata/core/util/CarbonProperties.java Outdated
return Boolean.parseBoolean(configuredValue);
}

public int maxSIRepairLimit() {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to "getMaxSIRepairLimit"

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

kunal642
kunal642 reviewed Aug 26, 2020
View reviewed changes
.../scala/org/apache/spark/sql/secondaryindex/events/SILoadEventListenerForFailedSegments.scala Outdated
case loadMetaDetail: LoadMetadataDetails =>
if (loadMetaDetail.getSegmentStatus == SegmentStatus.MARKED_FOR_DELETE &&
val maxSegmentRepairLimit = CarbonProperties.getInstance().maxSIRepairLimit()
LOGGER.info("Number of Segments to be repaired: " + maxSegmentRepairLimit)
Copy link
Contributor

Choose a reason for hiding this comment

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

add tableUniqueName to the log as well

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

@CarbonDataQA1
Copy link

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

@CarbonDataQA1
Copy link

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

@kunal642
Copy link
Contributor

kunal642 commented Sep 1, 2020

please add the new properties in docs

@vikramahuja1001
Copy link
Contributor Author

Added new properties in docs as well

@CarbonDataQA1
Copy link

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

@CarbonDataQA1
Copy link

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

@kunal642
Copy link
Contributor

kunal642 commented Sep 2, 2020

@vikramahuja1001 please fix the build

@CarbonDataQA1
Copy link

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

@CarbonDataQA1
Copy link

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

@kunal642
Copy link
Contributor

kunal642 commented Sep 2, 2020

LGTM

@asfgit asfgit closed this in c05b69e Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kunal642 kunal642 kunal642 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vikramahuja1001 @CarbonDataQA1 @kunal642