HBASE-26323 introduce a SnapshotProcedure #3716
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
This is a good start.
Overall the approach is good. I think you should have learned a lot on on proc-v2 and am-v2 @frostruan . It is not an easy work. Really appreciate on your effect.
Left some comments, let's work together to get this done. There are some high level questions:
- How to prevent split/merge at the same time when the SnapshotProcedure only takes shared lock on the table?
- Why do we let client specify whether to use zk coordination?
- The RemoteProcedureRequest is not enough for snapshot request? It is designed to be general usage.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
Outdated
Show resolved
Hide resolved
hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto
Outdated
Show resolved
Hide resolved
hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto
Outdated
Show resolved
Hide resolved
| @@ -277,6 +284,7 @@ message ExecuteProceduresRequest { | |||
| repeated OpenRegionRequest open_region = 1; | |||
| repeated CloseRegionRequest close_region = 2; | |||
| repeated RemoteProcedureRequest proc = 3; | |||
| repeated SnapshotRegionRequest snapshot_region = 4; | |||
There was a problem hiding this comment.
It can not be implemented as a RemoteProcedure?
There was a problem hiding this comment.
ok. Do you think if we can introduce a snapshotTable method in region server for master to snapshot online regions?
There was a problem hiding this comment.
You can see the current implementation for some remote procedures. They just use the same executeProcedures method for sending from master to region server. The RemoteProcedureRequest has a class name field for creating it using reflection. You can implement the logic of snapshot in the created class.
There was a problem hiding this comment.
Thanks for your reply. @Apache9
I think we have three options to implement snapshot logic here.
option a. using a RegionRemoteProcedure to implement region related operations, like open, close, snapshot and flush. That's my choice in the first commit.
option b. using a ServerRemoteProcedure to snapshot regions (or just a single region) in a remote server, like log splitting or claim replication queue. That's my choice for snapshot verifying. I think this is what you recommend.
option c. we introduce a new method snapshotRegion in Admin.proto for RegionServerAdmin, and give it admin priority just like flush region and log roll. for master, we just throw unsupported exception.
Sorry I misunderstood what you say "It can not be implemented as a RemoteProcedure?". I thought you mean "we shouldn't using remote procedure to implement this". so I changed from option a to option c.
Of course we can implement snapshot with ServerRemoteProcedure. Actually I think it makes easier to snapshot regions because we don't need to modify RSProcedureDispatcher to introduce a new kind of region operation. I will try to fix this as soon as possible. Thank you.
| // but we may need to downgrade it to shared lock for some reasons: | ||
| // a. exclusive lock has a negative effect on assigning region. See HBASE-21480 for details. | ||
| // b. we want to support taking multiple different snapshots on same table on the same time. | ||
| if (env.getProcedureScheduler().waitTableSharedLock(this, getTableName())) { |
There was a problem hiding this comment.
Is this enough? The SplitTableRegionProcedure and MergeTableRegionsProcedure also take shared lock on table, so taking shared lock can not prevent split/merge at the same time.
There was a problem hiding this comment.
Thanks for reviewing @Apache9 . Yes, shared table lock is not enough, so there is some extra work in SnapshotManager. Before SplitTableRegionProcedure/MergeTableRegionsProcedure runs, they will check if the table is in snapshot. If table is in snapshot, the procedure will stop. In SnapshotManager, we have a map whose key is SnapshotDescription and value is SnapshotProcedure id. The map will be rebuild when master restarts. Would you mind taking a look on those?
There was a problem hiding this comment.
But what if a merge or a split is already on going when we want to start snapshot here?
| throw new UnsupportedOperationException("unhandled state=" + state); | ||
| } | ||
| } catch (Exception e) { | ||
| if (e instanceof CorruptedSnapshotException) { |
There was a problem hiding this comment.
Better add this catch to the specific state? We will jump to the beginning of the procedure, I do not think we can do this in later states?
There was a problem hiding this comment.
Yes, a new specific state would be better. I will try to fix it. Thanks for your advise @Apache9
There was a problem hiding this comment.
When snapshot is corrupted, I just mark the state of procedure as FAILED and rollback the procedure. Do you think if we can make the procedure retryable like SplitWALProcedure or SyncReplicationReplayWALProcedure? @Apache9
| new MasterSnapshotVerifier(env.getMasterServices(), snapshot, workingDirFS); | ||
|
|
||
| if (numRegions >= verifyThreshold) { | ||
| verifier.verifySnapshot(false); |
There was a problem hiding this comment.
We do not need to create the verifier for this case?
There was a problem hiding this comment.
sorry, code here is a little confusing. let me try to explain this.
There are four parts in snapshot verifying.
a. verify snapshot descriptor.
b. verify table descriptor.
c. verify region count and region info.
d. verify store fils.
For large table snapshot, most time will be spent on verifying region info and store files. The SnapshotVerifyProcedure was designed to only verify region info and store files, so here I create a master verifier to verify snapshot descriptor, table descriptor and region count.
Sure, we can let SnapshotVerifyProcedure do this work also. I will fix it. Thanks for your advise @Apache9 .
...e-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SnapshotRegionProcedure.java
Outdated
Show resolved
Hide resolved
...e-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SnapshotVerifyProcedure.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| // but we may need to downgrade it to shared lock for some reasons: | ||
| // a. exclusive lock has a negative effect on assigning region. See HBASE-21480 for details. | ||
| // b. we want to support taking multiple different snapshots on same table on the same time. | ||
| if (env.getProcedureScheduler().waitTableSharedLock(this, getTableName())) { |
There was a problem hiding this comment.
But what if a merge or a split is already on going when we want to start snapshot here?
| status.markComplete("Snapshot " + snapshot.getName() + " completed"); | ||
| } | ||
|
|
||
| private void snapshotOfflineRegions(MasterProcedureEnv env) throws IOException { |
There was a problem hiding this comment.
OK, so this is for snapshoting split parent. Better just name it snapshotSplitParentRegions, offline is a bit confusing here.
There was a problem hiding this comment.
Really missed this case. Thanks for reminding me. Split/Merge Procedure and Snapshot Procedure, these two procedures should check whether the other is executing before starting execution. Now we only check snapshot procedure before running Split/Merge Procedure. I will try to fix this.
| @Override | ||
| protected void serializeStateData(ProcedureStateSerializer serializer) throws IOException { | ||
| super.serializeStateData(serializer); | ||
| serializer.serialize(SnapshotProcedureStateData |
There was a problem hiding this comment.
We do not serialize snapshotManifest here? Then how do we restore it when recovery?
| @Override | ||
| protected void afterReplay(MasterProcedureEnv env) { | ||
| try { | ||
| prepareSnapshot(env); |
There was a problem hiding this comment.
OK, so the trick is here, we will call parepareSnapshot to restore snapshot manifest.
| try { | ||
| prepareSnapshot(env); | ||
| } catch (IOException e) { | ||
| LOG.error("Failed replaying {}, mark procedure as failed", this, e); |
There was a problem hiding this comment.
OK, so the rollback here is very simple, just delete the snapshot directory, so we are safe to mark it as failure at any time, no PONR, good.
| dispatched = false; | ||
| } | ||
|
|
||
| RegionStates regionStates = env.getAssignmentManager().getRegionStates(); |
There was a problem hiding this comment.
What if the table is disabled? We will not use SnapshotProcedure for disabled table?
There was a problem hiding this comment.
If table is disabled, we will derectly jump into state SNAPSHOT_SNAPSHOT_OFFLINE_REGIONS. However I didn't make disabled table snapshot distributed, just do the work on master side. It may be slow but it will not hurt the availability if table is disabled.
What do you think ?
| @Override | ||
| protected void complete(MasterProcedureEnv env, Throwable error) { | ||
| if (error != null) { | ||
| Throwable realError = error.getCause(); |
There was a problem hiding this comment.
Why we need this trick here? We will always wrap the actual exception?
There was a problem hiding this comment.
yes, on the master side we will wrap this into a RemoteProcedureException.
| } | ||
|
|
||
| private Optional<ServerName> newTargetServer(MasterProcedureEnv env) { | ||
| List<ServerName> onlineServers = |
There was a problem hiding this comment.
We have implemented a ReservoirSample class in hbase-common, you can this this class to do random selection, without copying all the online servers out.
There was a problem hiding this comment.
Ok. I will fix it. Thank you.
|
Really thanks for taking time review this PR. I am very happy to see someone still paying attention to this. @Apache9 Recently I realized that some of my previous ideas are problematic. For example
I have to say that this work is more complicated than I expected. I should post a design document first and split it into some sub tasks. This PR was submitted a little too early. I will post a design doc as soon as possible. Thanks again. @Apache9 |
|
It will be good to have a design doc first~ |
|
hi @Apache9 here is a simple doc, would you mind taking a look ? https://docs.google.com/document/d/1Il_PB1SenXGr1-mmCIWEogxEMeGZe2fpuN3bMbjqiGI/edit |
Suggest you send an email to the dev list to let more people review the design doc~ |
ok. Thank you very much. |
hi @Apache9 Thanks. |
|
Have you subscribed to the mailing list? |
yes. And here is the email I sent .....
|
|
Strange... |
|
never mind. I'll try other ways. Really thanks for your suggestions. @Apache9 |
|
Is it because only committers have the authority to send dev@hbase.apache.org emails? As a user, should I send emails to user@hbase.apache.org ? |
|
No, there is no such limitation. The only possible condition is whether you have subscribed to the mailing list... Anyway, you could send the content to me first and then I can help posting it to the mailing-list... Thanks |
|
Really thanks for your patience @Apache9 . anyway, I want to say Hi all, As we all know, currently the snapshot in hbase has a few limitations, so I want to propose a proc-v2 implementation of snapshot. Here are some related links. jira design doc the initial implementation If you are interested, please take a look in your free time. Looking forward your advice and feedback. Thanks. |
|
Email sent, could you please check whether you can see the email? Thanks. |
|
yes, I can. Thanks so much. |
No description provided.