-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update tutorial and example config files #42
base: master
Are you sure you want to change the base?
Changes from all commits
65d3e4e
f7f0cae
cdfdfc1
01feea4
a84db29
c2439cf
7405477
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,16 @@ | ||
{ | ||
"config": { | ||
"rdma_device": "", | ||
"rdma_device_port": 10005, | ||
"rdma_device_port": 10006, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Unresolved References to Old RDMA Device Port (10005) The old RDMA device port (10005) is still referenced in the following configuration files:
Please update these references to the new port number (10006) to ensure consistency across the codebase. 🔗 Analysis chainPlease clarify the reason for changing the RDMA device port. The RDMA device port has been changed from 10005 to 10006. While this change is straightforward, it's important to understand the reasoning behind it. Could you please provide more context on why this change was necessary? Additionally, it would be helpful to document this change in a comment or in the project's documentation to ensure other developers are aware of the new port number. To verify if this port change affects other parts of the system, you can run the following command: This will help identify any other files that might need to be updated with the new port number. Correction Needed: Update the search command for Python files. The previous command failed because the Adjust the search command to include both configuration and Python files correctly. The previous search did not find any references to 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any references to the old port number (10005) in the codebase
rg --type-add 'config:*.{json,yaml,yml,toml}' --type config --type python '10005'
Length of output: 112 Script: #!/bin/bash
# Search for any references to the old port number (10005) in the codebase
rg --type-add 'config:*.{json,yaml,yml,toml}' --type config -g '*.py' '10005'
Length of output: 226 Script: #!/bin/bash
# Search for any references to the old port number (10005) in configuration and Python files
rg --type-add 'config:*.{json,yaml,yml,toml}' --type=config -g '*.py' '10005'
rg --type-add 'config:*.{json,yaml,yml,toml}' --type=config '10005'
Length of output: 406 |
||
"resource_manager_address": "", | ||
"resource_manager_port": 0 | ||
}, | ||
"benchmark": { | ||
"memory": 256, | ||
"pin_threads": false, | ||
"repetitions": 100, | ||
"warmup_repetitions": 0, | ||
"numcores": 1, | ||
"hot_timeout": -1 | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,12 @@ | ||
{ | ||
"config": { | ||
"rdma_device": "", | ||
"rdma_device_port": 10000, | ||
"rdma_device_port": 10005, | ||
"node_name": "exec-mgr-node", | ||
"resource_manager_address": "", | ||
"resource_manager_port": 0, | ||
"resource_manager_secret": 0 | ||
"resource_manager_secret": 42, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enhance security for the resource manager secret. The
|
||
"rdma-sleep": true | ||
}, | ||
"executor": { | ||
"use_docker": false, | ||
|
@@ -13,4 +15,3 @@ | |
"pin_threads": false | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
{ | ||
"executors": [ | ||
{ | ||
"address": "", | ||
"port": 10000, | ||
"cores": 0 | ||
} | ||
] | ||
"executors": [ | ||
{ | ||
"node": "exec-mgr-node", | ||
"address": "", | ||
"port": 10005, | ||
"cores": 1, | ||
"memory": 512 | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,11 @@ | ||
{ | ||
"config": { | ||
"rdma_device": "", | ||
"rdma_device_port": 0, | ||
"http_network_address": "", | ||
"http_network_port": 0 | ||
"rdma_device_port": 10000, | ||
"http_network_address": "0.0.0.0", | ||
"http_network_port": 5000, | ||
"rdma-threads": 1, | ||
"rdma-secret": 42, | ||
"rdma-sleep": true | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,12 +77,13 @@ namespace rfaas { | |
~executor(); | ||
|
||
executor(executor&& obj); | ||
executor& operator=(executor&& obj); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure consistent implementation of special member functions in You have added a move assignment operator |
||
|
||
bool connect(const std::string & ip, int port); | ||
|
||
// Skipping managers is useful for benchmarking | ||
bool allocate(std::string functions_path, int max_input_size, int hot_timeout, | ||
bool skip_manager = false, rdmalib::Benchmarker<5> * benchmarker = nullptr); | ||
bool skip_manager = false, bool skip_resource_manager = false, rdmalib::Benchmarker<5> * benchmarker = nullptr); | ||
void deallocate(); | ||
Comment on lines
+86
to
87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Incomplete Updates to Several calls to
🔗 Analysis chainVerify updates to all calls of The Run the following script to locate all calls to 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Find all calls to `executor::allocate` and display their usage.
rg --type cpp 'executor\.allocate\(' -A 2
Length of output: 1627 |
||
rdmalib::Buffer<char> load_library(std::string path); | ||
void poll_queue(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,13 +122,15 @@ namespace rfaas { | |
int rcv_buf, int max_inline_data): | ||
_address(address), | ||
_port(port), | ||
_active(_address, _port, rcv_buf), | ||
_rcv_buf_size(rcv_buf), | ||
_send_buffer(1), | ||
_receive_buffer(rcv_buf), | ||
_max_inline_data(max_inline_data) | ||
{ | ||
_active.allocate(); | ||
if(!address.empty()) { | ||
_active = rdmalib::RDMAActive(_address, _port, rcv_buf); | ||
_active.allocate(); | ||
} | ||
Comment on lines
+130
to
+133
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Issues Found with The conditional initialization of Key Points to Address:
🔗 Analysis chainLGTM, but consider additional safeguards. The conditional initialization of However, please consider the following:
To ensure that all usages of This will help identify any potential issues with unguarded 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Find all usages of _active in the resource_mgr_connection class
# Test: Search for _active usages in resource_mgr_connection methods
ast-grep --lang cpp --pattern $'class resource_mgr_connection {
$$$
$_($_) {
$$$
_active$_
$$$
}
$$$
}'
Length of output: 469 Script: #!/bin/bash
# Description: Find all usages of _active in the resource_mgr_connection class with a corrected ast-grep pattern
ast-grep --lang cpp --pattern $'class resource_mgr_connection {
$$$
$$method_declaration {
$$$
_active$$$
$$$
}
$$$
}'
Length of output: 484 Script: #!/bin/bash
# Description: Find all usages of _active in the resource_mgr_connection class using ripgrep
# Search for '_active' within the resource_mgr_connection class
rg '_active' rfaas/lib/connection.cpp --context 2
Length of output: 3346 |
||
} | ||
|
||
bool resource_mgr_connection::connect() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider refactoring duplicated error handling in resource leasing
Both branches of the
if
statement handle leasing an executor and check for failure in a similar way. To improve maintainability and reduce code duplication, consider extracting common error handling into a separate function or restructuring the logic.