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-3920]Fix compaction failure issue for SI table and metadata mismatch in concurrency #3854

Closed
wants to merge 1 commit into from
Closed

[CARBONDATA-3920]Fix compaction failure issue for SI table and metadata mismatch in concurrency #3854

wants to merge 1 commit into from

Conversation

akashrn5
Copy link
Contributor

@akashrn5 akashrn5 commented Jul 20, 2020 •
edited

Why is this PR needed?

  1. When load and compaction are happening concurrently, in reliability test segment data will be deleted from SI table, which leads to exception/failures
  2. pre-priming was happening for SI table segment in case of compaction before making SI segment as a success.

What changes were proposed in this PR?

  1. remove unnecessary cleaning API call from SI flow and before compaction success segment locks were getting released for SI, handle that
  2. do the code refactoring in case of SI load after main table compaction to handle proper pre-priming after segments were made success.

Does this PR introduce any user interface change?

  • No

Is any new testcase added?

  • No(tested in cluster with 10 concurrency and around 1000 loads)

@CarbonDataQA1
Copy link

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

@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/1697/

@akashrn5 akashrn5 changed the title [WIP]Fix compaction failure issue for SI table and metadata mismatch in concurrency [CARBONDATA-3920]Fix compaction failure issue for SI table and metadata mismatch in concurrency Jul 22, 2020
@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/1729/

@CarbonDataQA1
Copy link

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

ajantha-bhat
ajantha-bhat reviewed Jul 23, 2020
View reviewed changes
.../src/main/scala/org/apache/spark/sql/secondaryindex/events/CleanFilesPostEventListener.scala
var mainTableLocked = false
var indexTableLocked = false
try {
mainTableLocked = mainTableStatusLock.lockWithRetries()
Copy link
Member

Choose a reason for hiding this comment

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

If unable to get lock during the concurrent scenario, better to throw an exception to retry clean files command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since its a clean files, no need to throw exception, it can retry next time

Copy link
Member

Choose a reason for hiding this comment

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

Atleast add an error log of unable to get lock, so that the user will know that something happened and need to retry.

User tries multiple times in concurrent scenario and it won't clean due to lock issue. He will never know why it is not cleaned.

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

ajantha-bhat
ajantha-bhat reviewed Jul 23, 2020
View reviewed changes
.../src/main/scala/org/apache/spark/sql/secondaryindex/events/CleanFilesPostEventListener.scala Outdated
detail.setSegmentStatus(segToStatusMap(detail.getLoadName))
detail.setVisibility("false")
}
indexTableStatusLock.unlock()
Copy link
Member

Choose a reason for hiding this comment

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

release the lock after updating the SI table status. now it is released before. It can impact concurrent scenarios

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

ajantha-bhat
ajantha-bhat reviewed Jul 23, 2020
View reviewed changes
.../src/main/scala/org/apache/spark/sql/secondaryindex/events/CleanFilesPostEventListener.scala Outdated
detail.setSegmentStatus(segToStatusMap(detail.getLoadName))
detail.setVisibility("false")
}
CarbonInternalLoaderUtil.recordLoadMetadata(
Copy link
Member

Choose a reason for hiding this comment

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

This will fail as it will try to acquire lock and we didn't release.

here we need to call directly writeLoadDetailsIntoFile, as we already have a lock.

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/3483/

@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/1741/

@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/1743/

@CarbonDataQA1
Copy link

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

@akashrn5
Copy link
Contributor Author

@ajantha-bhat please review and merge

@ajantha-bhat
Copy link
Member

LGTM

@asfgit asfgit closed this in 30eefe3 Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajantha-bhat ajantha-bhat ajantha-bhat 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
@akashrn5 @CarbonDataQA1 @ajantha-bhat