Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6511 Deletes fail in case of failed region split - #1266
Merged
Conversation
abhishek-chouhan
commented
Jul 13, 2021
ContributorAuthor
Note, this impacts 4.x only. 5.x has a different logic wrt. to handling this. In hbase2.x the split hooks got moved to master observer since master handles the split through procedures, hence only region close is blocked by the lock in UngroupedAggRegionObserver. In case of a region close, if an error is encountered RS is aborted which results in resetting the state and hence is not a problem. |
dbwong
commented
Jul 13, 2021
Contributor
Quick question i guess the assumption is if we are post split we can't be splitting again? Does a hbase state machine explicitly prevent this or is the assumption that there is no way a flush or set of flushes could cause this to be splitting again? |
abhishek-chouhanforce-pushed
the
PHOENIX-6511
branch
from
July 13, 2021 20:08
db60338 to
8fc6019Compareabhishek-chouhan
commented
Jul 13, 2021
ContributorAuthor
@dbwong There was a small window between deleting the exclusive lock for region splitting and execution of the post hook. I've changed the PR so that we do our logic in the preRollbackSplit hook, which is executed just before deleting the lock. |
stoty
commented
Jul 13, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
abhishek-chouhanforce-pushed
the
PHOENIX-6511
branch
from
July 13, 2021 21:08
8fc6019 to
3d2f969Comparedbwong
approved these changes
Jul 13, 2021
stoty
commented
Jul 14, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jul 14, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
virajjasani
approved these changes
Jul 14, 2021
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
Left a couple of nits, +1 overall
Comment on lines
+90
to
+92
| ObserverContext ctx = new ObserverContext<>(null); | ||
| ctx.prepare(region.getCoprocessorHost().findCoprocessorEnvironment( | ||
| UngroupedAggregateRegionObserver.class.getName())); |
Contributor
There was a problem hiding this comment.
nit: Since preSplit and preRollBackSplit requires ObserverContext<RegionCoprocessorEnvironment> specifically, we can define ObserverContext accordingly here:
ObserverContext<RegionCoprocessorEnvironment> ctx = new ObserverContext<>(null);
ctx.prepare((RegionCoprocessorEnvironment) region.getCoprocessorHost().findCoprocessorEnvironment(
UngroupedAggregateRegionObserver.class.getName()));
| setUpConfigForMiniCluster(conf); | ||
| hbaseTestUtil.startMiniCluster(); | ||
| // establish url and quorum. Need to use PhoenixDriver and not PhoenixTestDriver | ||
| zkQuorum = "localhost:" + hbaseTestUtil.getZkCluster().getClientPort(); |
Contributor
There was a problem hiding this comment.
nit: could be a local variable than a class level field
abhishek-chouhanforce-pushed
the
PHOENIX-6511
branch
from
July 14, 2021 23:57
3d2f969 to
b40ad14Compareabhishek-chouhan
commented
Jul 15, 2021
ContributorAuthor
Thanks for the reviews @virajjasani@dbwong@kadirozde |
stoty
commented
Jul 15, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
| UngroupedAggregateRegionObserver obs = | ||
| (UngroupedAggregateRegionObserver) region.getCoprocessorHost() | ||
| .findCoprocessor(UngroupedAggregateRegionObserver.class.getName()); | ||
| ObserverContext<RegionCoprocessorEnvironment> ctx = new ObserverContext<>(null); |
Contributor
There was a problem hiding this comment.
4.x still support jdk1.7 now, seems compile broken:
UngroupedAggregateRegionObserverSplitFailureIT.java:[91,69] cannot infer type arguments for org.apache.hadoop.hbase.coprocessor.ObserverContext<>
[ERROR] reason: cannot infer type-variable(s) E
[ERROR] (actual and formal argument lists differ in length)
ContributorAuthor
There was a problem hiding this comment.
Thanks for pointing this out. Turns out that the constructor is different between 1.4 and later. Fixed the issue now y using compatObserverContext
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.