> On Nov. 21, 2012, 12:52 a.m., Ben Mahler wrote: > > src/common/resources.hpp, line 281 > > <https://reviews.apache.org/r/8145/diff/1/?file=222293#file222293line281> > > > > This vector of pairs looks a bit rough for a caller to use. > > > > 1. Any reason to not use uint16_t? It is the true size of port numbers, > > using uint64_t might require some error handling on the caller. > > > > 2. Could we just use a std::set or hashset of the ports available? > > > > This might be a bit high on the memory front: > > maximum size = (65536 ports * 2 bytes / 1024) = 128KB + overhead. > > But expected case would definitely be no more than a quarter of that. > > > > I think we should do this for simplicity. > > > > Longer term, if performance / memory warrants it, we could have a > > Ranges abstraction that only stores the ranges and provides set membership > > checks? > > Vinod Kone wrote: > I used uint64_t because thats what 'Range' takes in the protobuf. But I > guess I can just down convert 64bit numbers to 32bit when getting ports. > > Regarding the 2nd point, I did consider using a list of ports, but > disregarded it because of its memory inefficiency. All our operations on > ranges just work on > ranges without converting them to a flat list. But, I agree on providing > set membership abstractions for ranges. We can probably extend our 'ranges' > namespace in values.cpp. Not part of this review though.
Ok not part of this review then if you don't want to have to change the range logic. P.S. Another easy way out could be to use a std::bitset instead of std::set, the bitset would use 8KB, which isn't that bad. Ironically, I remember hearing of some bugs around manipulating the ranges? Like port allocation being buggy? If so, that would make me vote towards bundling the ranges abstraction in this review or the next. Your choice. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8145/#review13654 ----------------------------------------------------------- On Nov. 21, 2012, 2:57 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8145/ > ----------------------------------------------------------- > > (Updated Nov. 21, 2012, 2:57 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Added helpers to extract specific resources: cpus,mem, disk and ports. > > I wanted these as part of a fix I am going to make to the slave, to properly > deal with resources specified via command line. > > Hopefully, once we make the resources first class, we don't need these > helpers? > > > Diffs > ----- > > src/common/resources.hpp 5237b6031919aaed2d742bc095e537e83bf9f3fd > src/tests/resources_tests.cpp 83e93482a6800724d51aeae91e57d2a2b52b37b6 > > Diff: https://reviews.apache.org/r/8145/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
