Closed monidzik closed 5 years ago
@daggarwa it's still a draft since I need to check two more things:
But feel free to start a review.
@daggarwa it's still a draft since I need to check two more things:
- save variance and rusage values to database
- correctly transfer std::chrono::duration variables to odb
But feel free to start a review.
@monidzik Okay I will get to this either today or tomorrow.
@daggarwa it's still a draft since I need to check two more things:
- save variance and rusage values to database
- correctly transfer std::chrono::duration variables to odb
But feel free to start a review.
@monidzik Is this done yet?
@monidzik In general , I would recommend to keep the commits atomic. By atomic I mean that each commit can successfully compile locally and on CI and also can include one feature/sub feature completely. Like for instance a sample breakdown could be :
Something like this. It would help in quicker isolation of bugs and also in rollbacks etc. Otherwise awesome job on this ! :) This would help us immensely to track the performance over time for different DDS implementations.
@daggarwa https://github.com/ApexAI/performance_test/pull/84#issuecomment-536045571 yes, it's done: variance and correct chrono values are stored in the database
@daggarwa I addressed all you comments, back to you. I haven't clean up the commits yet, but I think it will be the last step after yours and Andreas's reviews.
@deeplearningrobotics I guess Monika has addressed almost all my comments except two small changes. Transferring over the PR to you for review. Thanks.
@deeplearningrobotics I addressed all your comments, so back to you
@deeplearningrobotics I changed those 3 things you commented above, is there something more than cleaning up the commits here to be done?
@monidzik: The code looks good aside from one bug.
@daggarwa: After the polling subscription is tested by @monidzik and merged can you rebased this branch on master and the test it. After this we can merge it.
@monidzik: The code looks good aside from one bug.
@daggarwa: After the polling subscription is tested by @monidzik and merged can you rebased this branch on master and the test it. After this we can merge it.
@deeplearningrobotics What is the bug in discussion? I have to discuss with you on utilizing the perf
tool for identifying performance bottleneck if any for polling subscription plugin. That is the only thing left there.
@daggarwa I think @deeplearningrobotics meant disabling foreign keys in analyze_runner.cpp, right? I first added the comment why I'm disabling foreign keys, but then I tested couple of times without disabling them and it all worked well... so I assume you don't have to do this anymore. But Divya, when you test it, make sure you don't have any issues in SQLite while creating database schema.
The user will be able to additionally save the results into a MySQL, SQLite or PostgreSQL database using odb object-relational mapping for C++. The code can be extended to other types of databases. I haven't check the Postgre database yet, but SQLite and MySQL should work well. Divya, please go through a code and try it out if it works correctly.
This change is