Conversation
igchor
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @karczex)
examples/mpsc_queue/mpsc_queue.cpp, line 49 at r1 (raw file):
[&](pmem::obj::experimental::mpsc_queue::batch_type rd_acc) { for (pmem::obj::string_view str : rd_acc) { std::cout << str.data() << std::endl;
Problem with printing garbage is that there is no null terminator. This is once again problem with no operator<< for string_view.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 37 at r1 (raw file):
* In case of crash or shutdown, reading and writing may be continued * by new process, without loss of any, already produced data. *
Please add that, try_consume_batch MUST be called after restart (otherwise produce might fail even if the queue is empty).
Codecov Report
@@ Coverage Diff @@
## master #1119 +/- ##
==========================================
- Coverage 94.24% 94.22% -0.02%
==========================================
Files 52 52
Lines 5159 5159
==========================================
- Hits 4862 4861 -1
- Misses 297 298 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
karczex
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @igchor)
include/libpmemobj++/experimental/mpsc_queue.hpp, line 37 at r1 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Please add that, try_consume_batch MUST be called after restart (otherwise produce might fail even if the queue is empty).
Done.
igchor
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @karczex)
examples/mpsc_queue/CMakeLists.txt, line 5 at r2 (raw file):
cmake_minimum_required(VERSION 3.3) project(inline_string CXX)
inline_string?
include/libpmemobj++/experimental/mpsc_queue.hpp, line 322 at r2 (raw file):
/** * Evaluates function f for the data, which is ready to be consumed.
Please also mention that try_consume_batch starts transaction internally (user callback is called within a transaction).
include/libpmemobj++/experimental/mpsc_queue.hpp, line 330 at r2 (raw file):
* * see also: mpsc_queue::worker::try_produce() *
@throws transaction_scope_error ...
KFilipek
left a comment
There was a problem hiding this comment.
Reviewed 2 of 4 files at r1, 1 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @karczex)
lukaszstolarczuk
left a comment
There was a problem hiding this comment.
Reviewed 2 of 4 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 30 unresolved discussions (waiting on @karczex)
examples/CMakeLists.txt, line 49 at r2 (raw file):
# Add developer checks add_cppstyle(examples ${CMAKE_CURRENT_SOURCE_DIR}/*/*.*pp) add_check_whitespace(examples ${CMAKE_CURRENT_SOURCE_DIR}/*.*)
you've missed on level - /*
examples/mpsc_queue/mpsc_queue.cpp, line 39 at r2 (raw file):
pmem::obj::persistent_ptr<root> proot = pop.root(); /* Create mpsc_queue runtime, which uses pmem_log_type object to store
at runtime (?)
I'm not sure what you had in mind
examples/mpsc_queue/mpsc_queue.cpp, line 43 at r2 (raw file):
auto queue = pmem::obj::experimental::mpsc_queue(*proot->log, 1); /* Consume data, which was left in the queue from the previous run of
left ... from the prev. -> stored ... in the prev.
examples/mpsc_queue/mpsc_queue.cpp, line 44 at r2 (raw file):
/* Consume data, which was left in the queue from the previous run of * application */
the application
examples/mpsc_queue/mpsc_queue.cpp, line 78 at r2 (raw file):
}); } //! [try_produce_string_view]
did you check that nested snippets are working flawlessly?
examples/mpsc_queue/mpsc_queue.cpp, line 79 at r2 (raw file):
} //! [try_produce_string_view] /* Porduce data to be consumed in next run of application. */
Porduce misspell
examples/mpsc_queue/mpsc_queue.cpp, line 79 at r2 (raw file):
} //! [try_produce_string_view] /* Porduce data to be consumed in next run of application. */
the application
include/libpmemobj++/experimental/mpsc_queue.hpp, line 3 at r2 (raw file):
// SPDX-License-Identifier: BSD-3-Clause /* Copyright 2021, Intel Corporation */
pls add \file description
include/libpmemobj++/experimental/mpsc_queue.hpp, line 35 at r2 (raw file):
* queue. * * In case of crash or shutdown, reading and writing may be continued
continued from the last position...? pls extend
include/libpmemobj++/experimental/mpsc_queue.hpp, line 38 at r2 (raw file):
* by new process, without loss of any, already produced data. * * note: try_consume_batch() MUST be called after restart (otherwise produce
what for must it be called? pls extend
include/libpmemobj++/experimental/mpsc_queue.hpp, line 38 at r2 (raw file):
* by new process, without loss of any, already produced data. * * note: try_consume_batch() MUST be called after restart (otherwise produce
after restart of what?
include/libpmemobj++/experimental/mpsc_queue.hpp, line 101 at r2 (raw file):
public: /** * Helper type, which allows to iterate over mpsc_queue's ranges via
helper type doesn't sound well, imho ;)
perhaps write it similar to below: Type representing...
include/libpmemobj++/experimental/mpsc_queue.hpp, line 118 at r2 (raw file):
/** * mpsc_queue producer worker class. * note: All workers should be destroyed before destruction of
should -> have to
include/libpmemobj++/experimental/mpsc_queue.hpp, line 121 at r2 (raw file):
* mpsc_queue * * see also: mpsc_queue:try_produce_batch()
Doxygen syntax: \see MyOtherClass::alloc()
include/libpmemobj++/experimental/mpsc_queue.hpp, line 154 at r2 (raw file):
* mpsc_queue. * * Object of this type have to be managed by pmem::obj::pool, to be
has to be
include/libpmemobj++/experimental/mpsc_queue.hpp, line 178 at r2 (raw file):
* * @param[in] pmem reference to already allocated pmem_log_type object * @param[in] max_workers maximum number of workers witch may be used by the
witch -> which
include/libpmemobj++/experimental/mpsc_queue.hpp, line 178 at r2 (raw file):
* * @param[in] pmem reference to already allocated pmem_log_type object * @param[in] max_workers maximum number of workers witch may be used by the
by the runtime? not in the runtime?
include/libpmemobj++/experimental/mpsc_queue.hpp, line 178 at r2 (raw file):
* * @param[in] pmem reference to already allocated pmem_log_type object * @param[in] max_workers maximum number of workers witch may be used by the
what do you mean may be used? if I try to create more than that, then what?
include/libpmemobj++/experimental/mpsc_queue.hpp, line 272 at r2 (raw file):
* Constructs pmem_log_type object * * @param size size of the log
in what units?
include/libpmemobj++/experimental/mpsc_queue.hpp, line 280 at r2 (raw file):
/** * Returns string_view of the log, which allows to read-only access to the
what do you mean by the string_view of the log? pls clarify
include/libpmemobj++/experimental/mpsc_queue.hpp, line 308 at r2 (raw file):
/** * Registers the producer worker. Number of workers have to bo lesser or equal
less than or equal to
include/libpmemobj++/experimental/mpsc_queue.hpp, line 309 at r2 (raw file):
/** * Registers the producer worker. Number of workers have to bo lesser or equal * than specified in the mpsc_queue constructor.
specified what? pls extend e.g. ... max_workers specified in...
include/libpmemobj++/experimental/mpsc_queue.hpp, line 460 at r2 (raw file):
* fail. Any exception thrown from f will result in terminate(). * * @return true if f was evaluated, all data copied by it saved in the
were saved
include/libpmemobj++/experimental/mpsc_queue.hpp, line 461 at r2 (raw file):
* * @return true if f was evaluated, all data copied by it saved in the * mpsc_queue, and visible for the consumer.
and are visible
include/libpmemobj++/experimental/mpsc_queue.hpp, line 463 at r2 (raw file):
* mpsc_queue, and visible for the consumer. * * see also: try_consume_batch()
\see
include/libpmemobj++/experimental/mpsc_queue.hpp, line 510 at r2 (raw file):
* the data is visible for the consumer. By default do nothing. * * @return true if f was evaluated, all data copied by it saved in the
.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 639 at r2 (raw file):
/** * Returns an iterator to the beginning of the accessed range of the
accessed range? what do you mean?
karczex
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 30 unresolved discussions (waiting on @igchor, @karczex, @KFilipek, @lukaszstolarczuk, and @throws)
examples/CMakeLists.txt, line 49 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
you've missed on level -
/*
Done.
examples/mpsc_queue/CMakeLists.txt, line 5 at r2 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
inline_string?
Done.
examples/mpsc_queue/mpsc_queue.cpp, line 39 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
at runtime (?)
I'm not sure what you had in mind
Done.
examples/mpsc_queue/mpsc_queue.cpp, line 43 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
left ... from the prev.->stored ... in the prev.
Done.
examples/mpsc_queue/mpsc_queue.cpp, line 44 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
the application
Done.
examples/mpsc_queue/mpsc_queue.cpp, line 78 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
did you check that nested snippets are working flawlessly?
yes
examples/mpsc_queue/mpsc_queue.cpp, line 79 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
Porducemisspell
Done.
examples/mpsc_queue/mpsc_queue.cpp, line 79 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
the application
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 3 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
pls add
\filedescription
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 35 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
continued from the last position...? pls extend
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 38 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
what for must it be called? pls extend
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 38 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
after restart of what?
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 101 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
helper typedoesn't sound well, imho ;)perhaps write it similar to below:
Type representing...
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 118 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
should -> have to
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 121 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
Doxygen syntax:
\see MyOtherClass::alloc()
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 154 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
has to be
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 178 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
witch -> which
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 178 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
by the runtime? notin the runtime?
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 280 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
what do you mean by the
string_view of the log? pls clarify
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 308 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
less than or equal to
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 309 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
specified what? pls extend e.g.
... max_workers specified in...
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 322 at r2 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
Please also mention that try_consume_batch starts transaction internally (user callback is called within a transaction).
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 330 at r2 (raw file):
Previously, igchor (Igor Chorążewicz) wrote…
@throws transaction_scope_error ...
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 460 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
were saved
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 461 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
and are visible
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 463 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
\see
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 510 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
.
Done.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 639 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
accessed range? what do you mean?
I have no better idea how to describe it.
I changed batch_type description little bit, so I believe it's understandable now.
karczex
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 30 unresolved discussions (waiting on @igchor, @KFilipek, @lukaszstolarczuk, and @throws)
include/libpmemobj++/experimental/mpsc_queue.hpp, line 178 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
what do you mean
may be used? if I try to create more than that, then what?
That's tricky ;) as currently there is an assert.
include/libpmemobj++/experimental/mpsc_queue.hpp, line 272 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
in what units?
Done.
igchor
left a comment
There was a problem hiding this comment.
Reviewed 1 of 4 files at r3.
Reviewable status: 1 of 4 files reviewed, 27 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)
This change is