Uh oh!
There was an error while loading. Please reload this page.
HBASE-26323 introduce a SnapshotProcedure - #3716
Conversation
Apache-HBase
commented
Oct 3, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 3, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 3, 2021
💔 -1 overall
This message was automatically generated. |
Apache9
left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| 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 .
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Oct 6, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 6, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 6, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 1, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 1, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 1, 2021
🎊 +1 overall
This message was automatically generated. |
Apache9
left a comment
There was a problem hiding this comment.
Haven't finished reviewing the whole patch. It is so big...
The overall architecture is very good. But maybe there are still points which I haven't fully understand.
Need to discuss more.
| // 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.
frostruan
commented
Nov 27, 2021
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 |
Apache9
commented
Nov 27, 2021
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 |
Apache9
commented
Dec 5, 2021
Suggest you send an email to the dev list to let more people review the design doc~ |
frostruan
commented
Dec 5, 2021
ok. Thank you very much. |
frostruan
commented
Dec 6, 2021
hi @Apache9 Thanks. |
Apache9
commented
Dec 6, 2021
Have you subscribed to the mailing list? |
frostruan
commented
Dec 6, 2021
yes. And here is the email I sent .....
|
Apache9
commented
Dec 6, 2021
Strange... |
frostruan
commented
Dec 6, 2021
never mind. I'll try other ways. Really thanks for your suggestions. @Apache9 |
frostruan
commented
Dec 6, 2021
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 ? |
Apache9
commented
Dec 7, 2021
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 |
frostruan
commented
Dec 7, 2021
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. |
Apache9
commented
Dec 7, 2021
Email sent, could you please check whether you can see the email? Thanks. |
frostruan
commented
Dec 7, 2021
yes, I can. Thanks so much. |
No description provided.