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

Add the offset and size to the dump SST footer details #679

Merged

Conversation

git-hulk
Copy link
Contributor

Before applying this PR, the footer details:

Footer Details:
--------------------------------------
  metaindex handle: B0E499405C
  index handle: 8AC49940CD17
  table_magic_number: 9863518390377041911
  format version: 5

and after

Footer Details:
--------------------------------------
  metaindex handle: B0E499405C offset: 134640176 size: 92
  index handle: 8AC49940CD17 offset: 134636042 size: 3021
  table_magic_number: 9863518390377041911
  format version: 5

This closes #404

@udi-speedb udi-speedb self-requested a review September 21, 2023 08:52
@udi-speedb
Copy link
Contributor

@git-hulk Looks good.
Please just update HISTORY.md as well.
Thanks

@git-hulk
Copy link
Contributor Author

@udi-speedb Thanks for your review. I have updated the HISTORY.md, please take a look.

@git-hulk git-hulk force-pushed the add-offset-size-to-sst-dump-footer branch from b771bff to 14bafe2 Compare September 21, 2023 15:00
@udi-speedb
Copy link
Contributor

@git-hulk Thanks for that.

Just one last minor thing . Please add the issue's number to the end of the description you have added to the HISTORY.md like so:
* sst_dump: display metaindex_handle and the index_handle's offset and size in footer information (#404).

@git-hulk
Copy link
Contributor Author

@git-hulk Thanks for that.

Just one last minor thing . Please add the issue's number to the end of the description you have added to the HISTORY.md like so: * sst_dump: display metaindex_handle and the index_handle's offset and size in footer information (#404).

Sure, sorry for missing that.

@udi-speedb
Copy link
Contributor

@git-hulk Thanks for that.
Just one last minor thing . Please add the issue's number to the end of the description you have added to the HISTORY.md like so: * sst_dump: display metaindex_handle and the index_handle's offset and size in footer information (#404).

Sure, sorry for missing that.

No problem. Thanks a lot.

@udi-speedb
Copy link
Contributor

@git-hulk
Please squash your commits into a single one and force push.
The commit message of the first commit is fine as the commit message for the single, squashed commit. Just please also add the issue number at the end of that commit message:
Add the offset and size into the dump SST footer detail (#404)

Once done, I will approve the PR.

@git-hulk git-hulk force-pushed the add-offset-size-to-sst-dump-footer branch from 5f383e5 to f9341e4 Compare September 22, 2023 02:10
@git-hulk
Copy link
Contributor Author

@udi-speedb Done, thank you! I will take care of those rules next time.

@udi-speedb
Copy link
Contributor

@udi-speedb Done, thank you! I will take care of those rules next time.

@git-hulk Thanks a lot for your contribution.

@udi-speedb
Copy link
Contributor

@git-hulk I apologize, but I forgot to ask you to run make format on your code. So, please do and force push again.

@udi-speedb udi-speedb self-requested a review September 22, 2023 02:18
@git-hulk git-hulk force-pushed the add-offset-size-to-sst-dump-footer branch from f9341e4 to 2b85f05 Compare September 22, 2023 02:21
@git-hulk
Copy link
Contributor Author

git-hulk commented Sep 22, 2023

@git-hulk I apologize, but I forgot to ask you to run make format on your code. So, please do and force push again.

My bad, I should go through the CONTRIBUTING.md doc first before writing the code. Thanks for your guidance.

@udi-speedb
Copy link
Contributor

@git-hulk I apologize, but I forgot to ask you to run make format on your code. So, please do and force push again.

My bad, I should go through the CONTRIBUTING.md doc first before writing the code. Thanks for your guidance.

No problem. Thanks again for your cooperation and contribution.

@git-hulk
Copy link
Contributor Author

@udi-speedb Looks like the check history and license are not correct, should I fix it?

https://github.com/speedb-io/speedb/blob/main/.github/workflows/check_license_and_history.yml#L36

I guess it should checkout the repository before checking.

@udi-speedb
Copy link
Contributor

@udi-speedb Looks like the check history and license are not correct, should I fix it?

https://github.com/speedb-io/speedb/blob/main/.github/workflows/check_license_and_history.yml#L36

I guess it should checkout the repository before checking.

No need. It's a known issue. We need to resolve it on our side. Thanks

@git-hulk
Copy link
Contributor Author

@udi-speedb Should I worry about the failure of the test? It looks like didn't relate to PR's change.

@udi-speedb
Copy link
Contributor

@udi-speedb Should I worry about the failure of the test? It looks like didn't relate to PR's change.

Indeed seems totally unrelated to your change. I am handling this. Will let you know if anything else is needed. Thanks

@udi-speedb udi-speedb self-requested a review September 27, 2023 13:29
@git-hulk git-hulk force-pushed the add-offset-size-to-sst-dump-footer branch from 2b85f05 to a780635 Compare October 12, 2023 11:36
@udi-speedb udi-speedb self-requested a review October 12, 2023 12:50
@udi-speedb
Copy link
Contributor

@git-hulk I aplogize, but I need to ask you to update the license text at the top of the file you have modified. We need to update our CONTRIBUTING.md file to reflect this recent decision. You may find the text to add in this issue:
#713 .
As an example, you may see column_family.cc

Please do not force push your updates. Just add commits. At the time of merging, we will squash and leave only the applicable commit message.
Thanks for your cooperation.

@git-hulk
Copy link
Contributor Author

git-hulk commented Oct 12, 2023

Hi @udi-speedb It's my pleasure, thanks for you review. I think it's a good chance to learn the speedb develop flow.

@udi-speedb udi-speedb merged commit 6511a7d into speedb-io:main Oct 15, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sst_dump tool should show both the offset and size from the index block handle and metaindex block handle
2 participants