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

Refactor log_messages_bytes_limit parameter usage #949

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andreisilviudragnea
Copy link

@andreisilviudragnea andreisilviudragnea commented Apr 21, 2024

Problem

There are many functions with a long list of parameters and some of them can be removed by refactoring how those parameters are used. For example, log_messages_bytes_limit parameter can be retrieved directly from RuntimeConfig struct.

Summary of Changes

Replacing log_messages_bytes_limit parameter inside TransactionBatchProcessor::execute_loaded_transaction() with self.runtime_config.log_messages_bytes_limit allows removing many log_messages_bytes_limit parameters propagated across the codebase.

@mergify mergify bot requested a review from a team April 21, 2024 11:22
@andreisilviudragnea andreisilviudragnea force-pushed the refactor-log-messages-bytes-limit branch 3 times, most recently from 1c68b4d to f0feb94 Compare April 23, 2024 06:08
@andreisilviudragnea
Copy link
Author

@steviez What do you think about this?

@andreisilviudragnea andreisilviudragnea force-pushed the refactor-log-messages-bytes-limit branch from f0feb94 to 45334e7 Compare April 24, 2024 09:01
@andreisilviudragnea andreisilviudragnea requested review from a team as code owners April 24, 2024 09:01
@andreisilviudragnea andreisilviudragnea force-pushed the refactor-log-messages-bytes-limit branch from 45334e7 to 7d647fe Compare April 25, 2024 13:33
@andreisilviudragnea
Copy link
Author

@CriesofCarrots Do you think this PR could be helpful?

@buffalojoec
Copy link

Hey @andreisilviudragnea thanks for the contributions! I'd like to ask respectfully that you don't tag maintainers. One of us will definitely have a look at your proposed changes, we are just very busy!

Conceptually this looks like a nice cleanup. I'll look at this as soon as possible, unless someone beats me to it!

@buffalojoec buffalojoec added the CI Pull Request is ready to enter CI label Apr 25, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Apr 25, 2024
@andreisilviudragnea andreisilviudragnea force-pushed the refactor-log-messages-bytes-limit branch from 7d647fe to cfd0afc Compare April 25, 2024 17:32
@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Apr 26, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Apr 26, 2024
@andreisilviudragnea
Copy link
Author

andreisilviudragnea commented Apr 27, 2024

I see some failures in https://buildkite.com/anza/agave/builds/3584#018f1cb8-3593-42cd-9f49-c5cf61cb8fdf:

     Summary [ 530.880s] 5 tests run: 4 passed (3 slow), 1 failed, 48 skipped
        FAIL [ 247.644s] solana-local-cluster::local_cluster test_hard_fork_with_gap_in_roots

But they do not seem related to this PR, since the same test fails on master builds too (https://buildkite.com/anza/agave/builds/3583), but passes after some retries (https://buildkite.com/anza/agave/builds/3595).

I will rebase my PR and wait for CI tag to run CI again.

@andreisilviudragnea andreisilviudragnea force-pushed the refactor-log-messages-bytes-limit branch from cfd0afc to 140bcea Compare April 27, 2024 17:40
@steviez steviez removed request for a team April 27, 2024 20:43
@@ -4961,7 +4956,6 @@ impl Bank {
enable_return_data_recording: true,
},
&mut ExecuteTimings::default(),
Some(1000 * 1000),
Copy link

Choose a reason for hiding this comment

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

Generally, I like this change as we have lots of bloated argument lists in the codebase. That being said, this one looks like it might be a slight change of behavior.

  • Currently, callers of this function will always get a value of Some(1e6)
  • With your change, the value will be whatever is initialized in the first Bank (ie CLI arg)

It looks like we have this caller here; I'm not super familiar with BanksServer so will need to dig a little (or let someone else chime in) to ensure we're good with the change:

match bank.process_transaction_with_metadata(transaction) {

@andreisilviudragnea andreisilviudragnea force-pushed the refactor-log-messages-bytes-limit branch 3 times, most recently from df227a9 to f4798f0 Compare May 7, 2024 21:01
@andreisilviudragnea andreisilviudragnea force-pushed the refactor-log-messages-bytes-limit branch 2 times, most recently from a1515ed to 59dac8c Compare May 18, 2024 11:01
@andreisilviudragnea andreisilviudragnea force-pushed the refactor-log-messages-bytes-limit branch 2 times, most recently from 12f7274 to 64b48d3 Compare May 23, 2024 15:10
@andreisilviudragnea andreisilviudragnea force-pushed the refactor-log-messages-bytes-limit branch 2 times, most recently from 9511a45 to 975397d Compare June 1, 2024 08:49
Replacing `log_messages_bytes_limit` parameter inside `TransactionBatchProcessor::execute_loaded_transaction()` with `self.runtime_config.log_messages_bytes_limit` allows removing many `log_messages_bytes_limit` parameters propagated across the codebase.
@andreisilviudragnea andreisilviudragnea force-pushed the refactor-log-messages-bytes-limit branch from 975397d to a0eb94f Compare August 24, 2024 11:41
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.

5 participants