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.

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

HDDS-10295. Provide an "ozone repair" subcommand to update the snapshot info in transactionInfoTable #6533

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

DaveTeng0
Copy link
Contributor

What changes were proposed in this pull request?

The issue found in HDDS-9342 caused the snapshot info in OM transactionInfoTable not get updated timely, so that OM restart failed at update ID check during raft log reapply.

The recover solution is to find the largest update ID, and update the snapshot info in transactionInfoTable with this it.

The task aims to provide such an CLI to update the table. Be noted, the largest update ID and its term currently should still need manual find.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10295

How was this patch tested?

Integration test

@DaveTeng0
Copy link
Contributor Author

@dombizita dombizita changed the title Provide an ozone repair subcommand to update the snapshot info in transactionInfoTable HDDS-10295. Provide an "ozone repair" subcommand to update the snapshot info in transactionInfoTable Apr 16, 2024
@adoroszlai adoroszlai added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Apr 16, 2024
@errose28
Copy link
Contributor

The task aims to provide such an CLI to update the table. Be noted, the largest update ID and its term currently should still need manual find.

Since this is an offline CLI I think it should also support finding the largest updateID (even if it's slow) and doing the update. Maybe as two steps (one to find the largest ID, and another to update to that). Doing the repair incorrectly can result in some bad states and we should try to make the repair commands as safe as possible. @ChenSammi or @fapifta can probably confirm what the correct steps to do the repair are since I haven't actually manually repaired a DB from this bug myself. I think scanning the DB for largest update ID will give the correct number to set the transaction index to.

Copy link
Contributor

@hemantk-12 hemantk-12 left a comment

Choose a reason for hiding this comment

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

Thanks @DaveTeng0 for the patch.

Overall looks good to me, left some cosmetic comments.

@DaveTeng0
Copy link
Contributor Author

DaveTeng0 commented Apr 17, 2024

The task aims to provide such an CLI to update the table. Be noted, the largest update ID and its term currently should still need manual find.

Since this is an offline CLI I think it should also support finding the largest updateID (even if it's slow) and doing the update. Maybe as two steps (one to find the largest ID, and another to update to that). Doing the repair incorrectly can result in some bad states and we should try to make the repair commands as safe as possible. @ChenSammi or @fapifta can probably confirm what the correct steps to do the repair are since I haven't actually manually repaired a DB from this bug myself. I think scanning the DB for largest update ID will give the correct number to set the transaction index to.

hey @ChenSammi @szetszwo , I look at the previous jira https://issues.apache.org/jira/browse/HDDS-9342, but I'm still not sure what's the best way to retrieve the highest TermIndex, except checking om's log. I see that two maps of 'applyTransactionMap' and 'ratisTransactionMap' have been removed from om, which might contain that information. so do you know where we could retrieve that TermIndex information, other than looking at om's log?
Thanks!

@szetszwo
Copy link
Contributor

... two maps of 'applyTransactionMap' and 'ratisTransactionMap' have been removed from om ...

@DaveTeng0 , since this is an offline CLI, there is no OM running and these two maps are not available even if there were not removed.

@szetszwo
Copy link
Contributor

szetszwo commented Apr 17, 2024

... except checking om's log. ...

I guess you mean OM raft log? It also cannot be used since the log entries may or may not be applied.

The correct way is to fine the highest index from RocksDB. This should be what @errose28 has suggested.

@DaveTeng0
Copy link
Contributor Author

... two maps of 'applyTransactionMap' and 'ratisTransactionMap' have been removed from om ...

@DaveTeng0 , since this is an offline CLI, there is no OM running and these two maps are not available even if there were not removed.

oh! that's right!

@DaveTeng0
Copy link
Contributor Author

... except checking om's log. ...

I guess you mean OM raft log? It also cannot be used since the log entries may or may not be applied.

The correct way is to fine the highest index from RocksDB. This should be what @errose28 has suggested.

created a jira to investigate how to parse all RocksDB files to get latest highest TermIndex of OM. HDDS-10730

Copy link
Contributor

@hemantk-12 hemantk-12 left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

Thanks @DaveTeng0 added some comments for improved testing and usability.

@DaveTeng0
Copy link
Contributor Author

Hello! if no further new comments, please feel free to merge! Thanks!

@hemantk-12
Copy link
Contributor

hemantk-12 commented May 14, 2024

@DaveTeng0 TestTransactionInfoRepair tests are failing due to NPE. Can you please fix that?

Copy link
Contributor

@hemantk-12 hemantk-12 left a comment

Choose a reason for hiding this comment

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

LGTM+1

@hemantk-12
Copy link
Contributor

@errose28 can you please take a look at the final PR?

Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

Just a few more comments based on the latest iteration.

@Test
public void testUpdateTransactionInfoTable() throws Exception {
CommandLine cmd = new CommandLine(new RDBRepair()).addSubcommand(new TransactionInfoRepair());
String dbPath = OMStorage.getOmDbDir(conf) + OM_KEY_PREFIX + OM_DB_NAME;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. This is a path on the local filesystem, so it should be constructed from a Path or File object. OM_KEY_PREFIX is for files in the Ozone filesystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense! Changed to create a File object and retrieve its path. And changed to use pure "/" in test case instead.


String cmdOut2 = scanTransactionInfoTable(dbPath);
assertThat(cmdOut2).contains(testTerm + "#" + testIndex);
cluster.getOzoneManager().restart();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the goal to make sure that the OM starts correctly after the repair? If so, we should use the same transaction update command to restore the old values, then do a metadata write operation on the cluster when it comes back up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense! updated!

Comment on lines 89 to 90
System.err.println(TRANSACTION_INFO_TABLE + " is not in a column family in DB for the given path.");
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the command will still exit 0 in this case. If you throw something likeIllegalArgumentException the stack trace will be filtered out, the message printed, and the return code will be non-zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be tested in TestTransactionInfoRepair too.

Copy link
Contributor Author

@DaveTeng0 DaveTeng0 Jun 4, 2024

Choose a reason for hiding this comment

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

definitely makes sense, my mistake and I should have chose to throw exception here instead! thanks for catching it, and will add verification of the error message in test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
snapshot https://issues.apache.org/jira/browse/HDDS-6517
Projects
None yet
5 participants