Breakout huge test file and code cleanup #153
Conversation
er1cthe0ne
requested review from
xieus,
kevin-zhonghao,
haboy52581 and
cj-chung
| using grpc::Status; | ||
|
|
||
| static std::atomic<bool> running; | ||
| Aca_Async_GRPC_Server::~Aca_Async_GRPC_Server() |
There was a problem hiding this comment.
is this a legacy implementation?
There was a problem hiding this comment.
yes, that's the old implementation, not being compiled anymore.
| @@ -154,9 +154,6 @@ void Aca_Comm_Manager::print_goal_state(GoalState parsed_struct) | |||
| VpcConfiguration current_VpcConfiguration = | |||
| parsed_struct.vpc_states(i).configuration(); | |||
|
|
|||
| fprintf(stdout, "current_VpcConfiguration.format_version(): %d\n", | |||
There was a problem hiding this comment.
format version will come back in the form of ACA logger, right? :)
| @@ -104,8 +104,6 @@ int ACA_Dataplane_OVS::update_port_state_workitem(const PortState current_PortSt | |||
|
|
|||
| PortConfiguration current_PortConfiguration = current_PortState.configuration(); | |||
|
|
|||
| assert(current_PortConfiguration.format_version() > 0); | |||
There was a problem hiding this comment.
This asseration looks okay though.
| @@ -203,7 +203,6 @@ class GoalStateProvisionerClient { | |||
|
|
|||
| SubnetConfiguration *SubnetConiguration_builder = | |||
| new_subnet_states->mutable_configuration(); | |||
| SubnetConiguration_builder->set_format_version(1); | |||
There was a problem hiding this comment.
Could you elaborate a bit why we need to remove format version related codes and UT.
There was a problem hiding this comment.
Based on previous experience on newer DPM goal state send down to older ACA, when a field is added/removed inside a top level resource state (Subnet/Port/DHCP etc), it will shift the next fields and also next source state in a goal state message. Therefore, having format version in each resource state is not useful.
We will keep the format version on the top level GoalState message and use that one only:
message GoalState {
uint32 format_version = 1; <------- only use this one
repeated VpcState vpc_states = 2;
repeated SubnetState subnet_states = 3;
repeated PortState port_states = 4;
repeated NeighborState neighbor_states = 5;
repeated SecurityGroupState security_group_states = 6;
repeated DHCPState dhcp_states = 7;
repeated RouterState router_states = 8;
}
The final strategy will be addressed by futurewei-cloud/alcor#211, that could be:
message GoalState {
uint32 format_version = 1;
repeated VpcState vpc_states = 2;
repeated SubnetState subnet_states = 3;
repeated PortState port_states = 4;
repeated NeighborState neighbor_states = 5;
repeated SecurityGroupState security_group_states = 6;
repeated DHCPState dhcp_states = 7;
repeated RouterState router_states = 8;
ExtraState extra_state = 9; <-------- we can only add new fields at the end
}
There was a problem hiding this comment.
Make sense. As we collect more and more experiences on GS change along the way, I think backward- and forward- compatibility strategy becomes clearer. We should spend some time after 11/30 release to consolidate the idea and document it.
| using namespace aca_dhcp_server; | ||
| using namespace aca_dhcp_programming_if; | ||
|
|
||
| TEST(dhcp_config_test_cases, add_dhcp_entry_valid) |
There was a problem hiding this comment.
For each major category of UTs, can we add a few lines of comments?
There was a problem hiding this comment.
sure, that's easy, will add that before merging.
* Add pulsar dependencies and pulsar consumer * update aca_main.cpp with entry points to pulsar * rename MessageConsumer to MessagePulsarConsumer * fix aca_main.cpp * fix a wrong symbol * update gs_tests * update namespace and class name * a line of code was deleted by mistake * add producer and test * fix remaining bugs * resolve conflicts in Dockerfile * fix a bug * fix a bug * fix a bug * missing producer in cmakelists * fix a bug
…wei-cloud#153) * add one-box deployment script and k8s yaml file for ignite * add some test cases for the db module * db test case of multiple clients write the same record
This PR addresses futurewei-cloud/alcor#416
It include the following changes: