Problem: single validator benchmark can't run natively #1649
Conversation
single validator benchmark should be runnable locally, since there are no port issues temp
yihuang
force-pushed
the
fix-native-run
branch
from
b89c8d0 to
b1a3a88
Compare
Signed-off-by: yihuang <[email protected]>
Signed-off-by: yihuang <[email protected]>
yihuang
requested review from
mmsqe and
JayT106
and removed request for
a team
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1649 +/- ##
===========================================
- Coverage 34.94% 16.64% -18.30%
===========================================
Files 123 72 -51
Lines 11803 6212 -5591
===========================================
- Hits 4124 1034 -3090
+ Misses 7265 5055 -2210
+ Partials 414 123 -291 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
testground/benchmark/benchmark/transaction.py (2)
14-14: Approved changes with a suggestion for improvementThe changes improve code organization and maintainability:
- Importing
LOCAL_JSON_RPCfrom.utilscentralizes configuration.- Introducing
CONNECTION_POOL_SIZEas a constant improves readability and maintainability.Consider adding a comment explaining the rationale behind the
CONNECTION_POOL_SIZEvalue of 1024. This would help future maintainers understand if this value needs adjustment based on specific requirements or performance considerations.Also applies to: 17-17
Line range hint
1-133: Overall impact of changes is positive with a suggestion for further improvementThe modifications in this file contribute to the PR objectives by:
- Centralizing configuration with the
LOCAL_JSON_RPCimport.- Improving maintainability with the
CONNECTION_POOL_SIZEconstant.- Enhancing configurability of connection settings.
These changes should help in resolving port conflicts and enabling the single validator benchmark to run natively.
To further improve the code's flexibility, consider making
CONNECTION_POOL_SIZEconfigurable through environment variables or a configuration file. This would allow for easier adjustment of the connection pool size without code changes, which could be beneficial for different testing scenarios or environments.testground/benchmark/benchmark/utils.py (3)
16-16: LGTM. Consider using an environment variable for flexibility.The change from 'localhost' to '127.0.0.1' is acceptable and may help avoid potential DNS resolution issues. However, to improve flexibility and ease of configuration across different environments, consider using an environment variable for the RPC URL.
Here's a suggested improvement:
import os LOCAL_RPC = os.environ.get('LOCAL_RPC', "http://127.0.0.1:26657")This allows easy overriding of the RPC URL without modifying the code.
17-17: LGTM. Consider using an environment variable for consistency.The addition of
LOCAL_JSON_RPCimproves code maintainability by centralizing the JSON-RPC endpoint configuration. For consistency with the previous suggestion and to maintain flexibility across environments, consider using an environment variable for this URL as well.Here's a suggested improvement:
import os LOCAL_JSON_RPC = os.environ.get('LOCAL_JSON_RPC', "http://127.0.0.1:8545")This allows easy overriding of the JSON-RPC URL without modifying the code and maintains consistency with the
LOCAL_RPCvariable.
Line range hint
1-190: Summary: Changes align well with PR objectives and improve maintainability.The modifications in this file contribute effectively to the PR's goal of enabling the single validator benchmark to run natively without port conflicts. The centralization of RPC configurations improves maintainability and consistency across the codebase.
To further enhance flexibility and ease of configuration across different environments, consider implementing the suggested improvements using environment variables for both
LOCAL_RPCandLOCAL_JSON_RPC.Overall, these changes are well-implemented and support the PR objectives.
CHANGELOG.md (2)
5-7: Update the changelog entry as suggested in the previous review.The current entry:
* (testground)[1649](https://github.com/crypto-org-chain/cronos/pull/1649) Fix running single validator benchmark locally.Should be updated to:
* (testground)[#1649](https://github.com/crypto-org-chain/cronos/pull/1649) Fix running single validator benchmark locally.This change adds a
#before the pull request number, which is the standard format for GitHub issue/PR links.
Line range hint
1-1186: LGTM! Well-structured and informative changelog.The changelog is well-organized and provides detailed information about each release. The categorization of changes into "State Machine Breaking", "Improvements", and "Bug Fixes" helps users quickly understand the impact of each release.
For consistency, consider adding dates to all release sections, not just some of them. This would help users understand the timeline of releases more clearly.
testground/benchmark/benchmark/stateless.py (1)
198-203: Log exceptions instead of passing silentlyIn the
except OSError:block, the exception is silently ignored with apass. Logging the exception can aid in debugging and provide valuable information if unexpected issues occur.except OSError as e: # Ignore if the file is not writable when running in bare metal - pass + print(f"Failed to create tarball: {e}")Alternatively, consider using the
loggingmodule for more flexible logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- testground/benchmark/benchmark/stateless.py (6 hunks)
- testground/benchmark/benchmark/transaction.py (2 hunks)
- testground/benchmark/benchmark/utils.py (2 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint python
testground/benchmark/benchmark/stateless.py
[failure] 234-234:
./testground/benchmark/benchmark/stateless.py:234:5: BLK100 Black would make changes.
🔇 Additional comments (4)
testground/benchmark/benchmark/transaction.py (1)
130-133: Approved changes in thesendfunctionThe modification to use
CONNECTION_POOL_SIZEin theaiohttp.TCPConnectorinitialization improves code maintainability and consistency. This change aligns well with the goal of making the code more configurable and easier to maintain.testground/benchmark/benchmark/utils.py (1)
97-97: LGTM. Good use of the newLOCAL_JSON_RPCvariable.This change improves code maintainability by using the centralized
LOCAL_JSON_RPCconfiguration instead of a hardcoded URL. It's consistent with the earlier changes and supports the PR objective of enabling the single validator benchmark to run natively.testground/benchmark/benchmark/stateless.py (2)
7-7: Importing thesysmodule is appropriateThe
sysmodule is correctly imported to usesys.stdoutlater in the code.
234-236: Docstring formatting has been previously addressedThe formatting issue with the docstring for
generate_loadhas been noted in prior review comments.🧰 Tools
🪛 GitHub Check: Lint python
[failure] 234-234:
./testground/benchmark/benchmark/stateless.py:234:5: BLK100 Black would make changes.
Signed-off-by: yihuang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
testground/benchmark/benchmark/stateless.py (3)
198-210: Approve: Improved error handling for output file creationThe addition of the try-except block for tar file creation enhances the robustness of the code, particularly when running in environments where file writing might fail. This is a good improvement.
A minor suggestion to consider:
- You could log the error when the file is not writable to aid in debugging. For example:
except OSError as e: print(f"Warning: Unable to create output file: {e}", file=sys.stderr)This would provide more information about why the file couldn't be created without interrupting the program's execution.
230-243: Approve: Newgenerate_loadcommand with suggestion for improved documentationThe addition of the
generate_loadcommand is a valuable feature for manually generating load on an existing node. The implementation looks solid, utilizing theprepare_txsfunction and providing informative output.Suggestion for improvement:
- Enhance the docstring to provide more details about the command's purpose, usage, and parameters. For example:
def generate_load(datadir: Path, global_seq: int): """ Manually generate load for an existing node. This command prepares and sends transactions to an existing node, waits for a specified number of idle blocks, and then dumps block stats. Args: datadir (Path): Path to the data directory containing the config.json file. global_seq (int): Global sequence number for transaction generation. Usage: $ python stateless.py generate-load --datadir /path/to/data --global-seq 0 """ # ... (rest of the function remains the same)This improved docstring would make the command's purpose and usage clearer to other developers or users of the script.
433-442: Approve: Newprepare_txsfunction with suggestion for error handlingThe addition of the
prepare_txsfunction is a good improvement to the code structure. It encapsulates the logic for loading or generating transactions, making the code more modular and easier to maintain. The function is well-implemented and provides clear output about the transactions being prepared.Suggestion for improvement:
Consider adding basic error handling to make the function more robust. For example:def prepare_txs(cfg, datadir, global_seq): try: txs = transaction.load(datadir, global_seq) if txs: print("loaded", len(txs), "txs") else: print("generating", cfg["num_accounts"] * cfg["num_txs"], "txs") txs = transaction.gen( global_seq, cfg["num_accounts"], cfg["num_txs"], cfg["tx_type"] ) return txs except Exception as e: print(f"Error preparing transactions: {e}", file=sys.stderr) return []This addition would ensure that the function always returns a list of transactions, even if an error occurs during loading or generation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- testground/benchmark/benchmark/stateless.py (6 hunks)
🧰 Additional context used
🔇 Additional comments (2)
testground/benchmark/benchmark/stateless.py (2)
273-273: Approve: Refactored transaction preparationThe use of the
prepare_txsfunction to prepare transactions in thedo_runfunction is a good refactoring decision. This change:
- Centralizes the logic for transaction preparation, improving maintainability.
- Enhances code reusability, as the same function is used in both
do_runand the newgenerate_loadcommand.- Makes the code more modular and easier to understand.
This refactoring aligns well with the DRY (Don't Repeat Yourself) principle and improves the overall structure of the code.
7-7: Approve: Addition ofsysmodule importThe addition of the
sysmodule import is appropriate and necessary for the new functionality in thegenerate_loadfunction, where it's used to print block stats to stdout. This import is correctly placed at the beginning of the file with other standard library imports.
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
Signed-off-by: mmsqe <[email protected]>
85ca58b
single validator benchmark should be runnable locally, since there are no port issues
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation