Fix breeze kvstore nodes oputput #12
Conversation
| @@ -218,7 +218,7 @@ def _parse_nodes(rows, value): | |||
|
|
|||
| marker = '* ' if prefix_db.thisNodeName == host_id else '> ' | |||
| row = ["{}{}".format(marker, prefix_db.thisNodeName)] | |||
| row.extend(prefix_strs) | |||
| row.extend(map(lambda l: l.split()[0], prefix_strs.splitlines()[2:])) | |||
There was a problem hiding this comment.
Use newer syntax instead of lambda
row.extend([l.split()[0] for l in prefix_strs.splitlines()[2:]))
We should only print prefixes here which are of type LOOPBACK and use utils.sprint_prefix function to convert ip-prefix thrift object to string representation
// Get loopback prefixes
loopback_prefixes = [p.prefix for p in prefix_db.prefixEntries \
if p.prefixType = lsdb_types.PrefixType.LOOPBACK]
// add it to row
row.extend([utils.sprint_prefix(p) for p in loopback_prefixes])
There was a problem hiding this comment.
Can you also revise this pull request to follow our suggested template. Aim is that each pull request should clearly identify what change went into it and how it was tested.
https://github.com/facebook/openr/blob/master/.github/PULL_REQUEST_TEMPLATE.md
|
Also can you update issue to follow issue template. https://github.com/facebook/openr/blob/master/.github/ISSUE_TEMPLATE.md With clear indication of what is expected output and what should be actual output :) |
|
Thanks saifhhasan for the review and suggestion. Please check my new commit and updated comments, and let me know if further changes are necessary. |
| @@ -218,7 +218,9 @@ def _parse_nodes(rows, value): | |||
|
|
|||
There was a problem hiding this comment.
remove unnecessary variable prefix_strs as well.
|
Thanks again, removed |
There was a problem hiding this comment.
@saifhhasan is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@tamihiro has updated the pull request. View: changes, changes since last import |
|
@tamihiro has updated the pull request. View: changes, changes since last import |
|
@tamihiro has updated the pull request. View: changes, changes since last import |
|
@tamihiro has updated the pull request. View: changes, changes since last import |
Summary:
There is a race condition when KvStoreWrapper and ZMQ background threads are
destroyed. This diff is an attempt to explicitly destroy KvStoreWrapper in TearDown
Also use unique_ptr instead of shared_ptr
```
WARNING: ThreadSanitizer: data race (pid=49594)
Write of size 8 at 0x7ba000000220 by thread T1:
#0 close <null> (link_monitor_test+0xdb1f6a)
#1 zmq::signaler_t::~signaler_t() /home/engshare/third-party2/zeromq/4.3.1/src/zeromq-4.3.1/src/signaler.cpp:114:20 (link_monitor_test+0xbd1274)
Previous read of size 8 at 0x7ba000000220 by main thread:
#0 epoll_ctl <null> (link_monitor_test+0xd9bdef)
#1 epoll_del /home/engshare/third-party2/libevent/1.4.14b_hphp/src/libevent-1.4.14b-stable/epoll.c:485 (link_monitor_test+0xec4253)
#2 folly::EventBaseEvent::eb_event_del() <null> (link_monitor_test+0xa747ef)
#3 folly::EventHandler::~EventHandler() <null> (link_monitor_test+0xa74a70)
#4 std::_Hashtable<int, std::pair<int const, openr::OpenrEventBase::ZmqEventHandler>, std::allocator<std::pair<int const, openr::OpenrEventBase::ZmqEventHandler> >, std::__detail::_Select1st, std::equal_to<int>, std::hash<int>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::~_Hashtable() <null> (link_monitor_test+0x219620)
#5 openr::OpenrEventBase::~OpenrEventBase() <null> (link_monitor_test+0x212d8d)
#6 openr::KvStore::~KvStore() <null> (link_monitor_test+0x1bfbb7)
#7 openr::KvStore::~KvStore() <null> (link_monitor_test+0x1bfbe9)
#8 openr::KvStoreWrapper::~KvStoreWrapper() <null> (link_monitor_test+0x117533)
#9 std::_Sp_counted_ptr_inplace<openr::KvStoreWrapper, std::allocator<openr::KvStoreWrapper>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() <null> (link_monitor_test+0x116fbd)
#10 LinkMonitorTestFixture::~LinkMonitorTestFixture() <null> (link_monitor_test+0xf8285)
#11 LinkMonitorTestFixture_BasicOperation_Test::~LinkMonitorTestFixture_BasicOperation_Test() <null> (link_monitor_test+0xf7ee9)
#12 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) <null> (link_monitor_test+0xc4b2ae)
#13 __libc_start_main /home/engshare/third-party2/glibc/2.26/src/glibc-2.26/csu/libc-start.c:308:16 (libc.so.6+0x211a5)
```
Reviewed By: yi-xian
Differential Revision: D20148667
fbshipit-source-id: 71634c08df8ebbc98a6b8c4aa3329166834453f2
Summary: Pull Request resolved: facebookexperimental/rust-shed#12 The OpenSSL version on Mac doesn't work well with EdenSCM and Mononoke integration, just use the one from getdeps/brew. Also remove the now redundant "DEVELOPER_DIR" since the modern XCode version works. Pull Request resolved: facebook/sapling#63 Reviewed By: StanislavGlebik Differential Revision: D23927022 Pulled By: lukaspiatkowski fbshipit-source-id: 6b6b3baa33d49b567b9aa6178cbd20b7ae9edc89
Description:
breeze kvstore nodesreturned ugly output because the original code extended a list of string objects with a unicode object (prefix_strs).prefix_strs.Test Plan: