Skip to content

Conversation

@andreisilviudragnea
Copy link

@andreisilviudragnea andreisilviudragnea commented Apr 17, 2024

Problem

agave-ledger-tool supports --geyser-plugin-config option, but not --log-messages-bytes-limit, which can be useful for replaying ledger (using agave-ledger-tool verify, for example) to retrieve truncated logs for txs.

Summary of Changes

Added --log-messages-bytes-limit to all agave-ledger-tool supporting --geyser-plugin-config.

This should be backported to v1.17 and v1.18, because we need it for ledger snapshots created with v1.17.

@mergify mergify bot requested a review from a team April 17, 2024 06:59
@andreisilviudragnea andreisilviudragnea force-pushed the ledger-tool-log-messages-bytes-limit branch from 939ccd5 to ba2df18 Compare April 17, 2024 07:00
@andreisilviudragnea andreisilviudragnea force-pushed the ledger-tool-log-messages-bytes-limit branch from ba2df18 to 5456045 Compare April 17, 2024 08:03
@andreisilviudragnea andreisilviudragnea changed the title Add --log_messages_bytes_limit arg to ledger-tool Add --log-messages-bytes-limit arg to ledger-tool Apr 17, 2024
@steviez steviez added the CI Pull Request is ready to enter CI label Apr 17, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Apr 17, 2024
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Change looks fine, argument is identical to the one in validator bin which is ideal. One of these days we'll figure out a better solution to have all of those single-sourced. Added the CI label.

This should be backported to v1.17 and v1.18, because we need it for ledger snapshots created with v1.17.

Why exactly ? If this change lands in master, why can't you just use the tip of master ?

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (03b5965) to head (5456045).

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #854     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         851      851             
  Lines      231744   231744             
=========================================
- Hits       189692   189687      -5     
- Misses      42052    42057      +5     

@andreisilviudragnea
Copy link
Author

@steviez I am just following the guidelines from solana-bigtable README. I suppose it is needed to allow replaying the ledger with a version similar to the one when the ledger has been archived.

But it will still be a problem for all intermediate v1.17 versions, because only the latest one will have it (probably v1.17.32). I suppose people can just cherry pick these changes on their local checkout if needed, like I did for v1.17 here.

@steviez
Copy link

steviez commented Apr 17, 2024

I am just following the guidelines from solana-bigtable README.

Thanks for the link; while using the same version as what is listed in version.txt is the safest, it is not always strictly necessary. As of late, we have been striving to keep stable/beta/edge versions all consensus and ledger compatible. For example, if you used the latest v1.18 tag or tip of master on the latest epoch backup, I would expect identical replay results between v1.17.31 / v1.18.11 / master. More so, many of the release on any line will not have consensus altering changes. Ie, using v1.17.31 would likely be fine for an upload made from a v1.17.30 warehouse node

But it will still be a problem for all intermediate v1.17 versions, because only the latest one will have it (probably v1.17.32). I suppose people can just cherry pick these changes on their local checkout if needed, like I did for v1.17

Exactly, we can't go back in time and get this commit on all of the old releases. While I mentioned that v1.18 and master should™️ work, cherry-picking the commit on top of the exact tagged commit from the version.txt is probably the safest. This would also be required if you were loading up ledgers that were even older, for example, v1.16 timeframe.

In general, we try to limit backports to items that are deemed "critical" for the cluster (bug fixes, performance regressions, etc) which is why I was opposed to the BP's. We are obviously open to exceptions, but in this case, I think cherry-picking a single commit seems like a pretty reasonable alternative for you

Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution; forgot to hit a button that would allow a few more CI steps to start, letting those go now. Will merge on all greens

@steviez steviez merged commit 5906129 into anza-xyz:master Apr 17, 2024
@andreisilviudragnea andreisilviudragnea deleted the ledger-tool-log-messages-bytes-limit branch April 17, 2024 15:01
michaelschem pushed a commit to michaelschem/agave that referenced this pull request Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants