Extra timestamp tests #217

KFilipek · 2022-06-17T13:08:15Z

TODO:

refactor: entry_iterator class
prereq 1: (multiple regions, multithreaded synchronously appended entries)
- case 1: (in every region, all timestamps increase)
- case 2: (in stream, all timestamps increase (duplications))
prereq 2: (multiple regions, multithreaded asynchronously appended entries)
- case 1: (in every region, all timestamps increase)
- case 2: (in stream, all timestamps increase (duplications))
case 3: (multiple regions, remove single region with entries, add an empty region and then add new entries then check case 2)

Description from issue:

(Prereq 1) Add generator derivated by pmemstream_test_base for that case:

Generates multiple regions
Multithreaded synchronously append entries to regions
(Prereq 2) Add generator derivated by pmemstream_test_base for that case:

Generates multiple regions
Multithreaded asynchronously append entries to regions
Cases for (prereq 1 and 2):
Case 1:
In every region, all timestamps increase.

Case 2:
In stream, all timestamps increase (duplications).

Case 3:
Add regions, remove single region with entries, add an empty region and then add new entries then check Case 2.

Partially covers: #211

This change is

lukaszstolarczuk

Reviewable status: 0 of 4 files reviewed, 12 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)

tests/CMakeLists.txt line 146 at r2 (raw file):

	build_test_rc(NAME timestamp SRC_FILES unittest/timestamp.cpp LIBS miniasync)
	add_test_generic(NAME timestamp TRACERS none)

+ memcheck and pmemcheck

tests/common/stream_helpers.hpp line 23 at r2 (raw file):

 * different regions in global order. */
template <typename T>
class entry_iterator {

looks like these classes are copy-pasted from example, please add info about that in the commit msg

tests/common/stream_helpers.hpp line 23 at r2 (raw file):

 * different regions in global order. */
template <typename T>
class entry_iterator {

I think you can move this class into "pmem" namespace here

tests/common/stream_helpers.hpp line 36 at r2 (raw file):

		pmemstream_entry_iterator_seek_first(it.get());
		if (pmemstream_entry_iterator_is_valid(it.get()) != 0) {
			throw std::runtime_error("No entries to iterate");

should it throw when there are no entries...?

tests/common/stream_helpers.hpp line 623 at r2 (raw file):

	}

	bool validate_timestamps(bool possible_gaps = true)

you can ASSERT here already, no need for returning bool from the func

tests/common/stream_helpers.hpp line 629 at r2 (raw file):

		auto entry_iterators = entry_iterator<void>::get_entry_iterators(stream.c_ptr(), regions);

		uint64_t timestamp = (possible_gaps ? PMEMSTREAM_INVALID_TIMESTAMP : PMEMSTREAM_FIRST_TIMESTAMP);

why do we need this line?

tests/unittest/timestamp.cpp line 26 at r2 (raw file):

/* User data. */
struct payload {

why do we need this payload struct? we won't print the data in a pretty form within the test

tests/unittest/timestamp.cpp line 68 at r2 (raw file):

			"verify increasing timestamp values in each region after synchronous append",
			[&](pmemstream_test_base &&stream) {
				size_t no_elements = *rc::gen::inRange<size_t>(1, 15);

why do you need to limit the elems count?
if you really need it - you can use our shrinkable struct ranged instead

tests/unittest/timestamp.cpp line 69 at r2 (raw file):

			[&](pmemstream_test_base &&stream) {
				size_t no_elements = *rc::gen::inRange<size_t>(1, 15);
				size_t no_regions = *rc::gen::inRange<size_t>(1, TEST_DEFAULT_REGION_MULTI_MAX_COUNT);

you can use pmemstream_with_multi_empty_regions instead

tests/unittest/timestamp.cpp line 76 at r2 (raw file):

				/* Asynchronous append to many regions with global ordering */
				parallel_exec(no_regions, [&](size_t thread_id) {
					for (size_t i = 0; i < no_elements; i++) {

we have a helper append for this whole for loop

tests/unittest/timestamp.cpp line 88 at r2 (raw file):

				uint64_t prev_timestamp = PMEMSTREAM_INVALID_TIMESTAMP;
				size_t entry_counter = 0;
				for (auto e_iterator : entry_iterators) {

can't you use your new helper function for a single region, instead...?

tests/unittest/timestamp.cpp line 104 at r2 (raw file):

			});
	});
}

pls make the CI green, e.g. by providing a proper styling

igchor

Reviewable status: 0 of 4 files reviewed, 13 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

tests/common/stream_helpers.hpp line 23 at r2 (raw file):

 * different regions in global order. */
template <typename T>
class entry_iterator {

You can also add comment : "XXX replace with actual global entry iterator once implemented".

tests/common/stream_helpers.hpp line 623 at r2 (raw file):

	}

	bool validate_timestamps(bool possible_gaps = true)

nit: I would suggest splitting this function into 2 parts:

generate all entries in global order (use the entry_iterator class), you could create function similar to get_entries but which returns vector of pmemstream_entry
validate timestamps in those entries (e.g. if gaps are possible, you can just call std::is_sorted), otherwise you can .e.g compare the sequence with std::iota.

tests/unittest/timestamp.cpp line 88 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

can't you use your new helper function for a single region, instead...?

1

igchor

Reviewable status: 0 of 4 files reviewed, 13 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

tests/common/stream_helpers.hpp line 635 at r3 (raw file):

				entries.push_back(pmemstream_entry_iterator_get(oldest_iterator->raw_iterator()));
			} else {
				entry_iterators.erase(entry_iterators.begin() +

isn't it the same as entry_iterators.erase(oldest_iterator)?

tests/common/stream_helpers.hpp line 648 at r3 (raw file):

		std::vector<uint64_t> timestamps;
		std::transform(entries.begin(), entries.end(), timestamps.begin(),
			       [this](decltype(get_entries_for_regions(regions))::value_type entry) {

const auto& entry

tests/common/stream_helpers.hpp line 660 at r3 (raw file):

			return std::adjacent_find(timestamps.begin(), timestamps.end()) == timestamps.end();
		} else {
			std::vector<uint64_t> correct_timestamps(timestamps.size());

you can just do return std::adjacent_find(timestamps.begin(), timestamps.end()) == timestamps.end() && timetamps.front() == PMEMSTREAM_FIRST_TIMESTAMP && timestamps.back() == PMEMSTREAM_FIRST_TIMESTAMP + timestamps.size()

igchor

Reviewable status: 0 of 6 files reviewed, 14 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

tests/common/stream_helpers.hpp line 26 at r6 (raw file):

/* pmememstream entry iterator wrapper, which helps to manage entries from
 * different regions in global order. */
template <typename T>

Please remove the template, you don;t need it

tests/common/stream_helpers.hpp line 644 at r6 (raw file):

	}

	bool validate_timestamps(bool possible_gaps = true)

as we discussed, it would be nice to be able to pass some custom vector of entries/timestamps here

KFilipek

Reviewed 1 of 3 files at r2, 1 of 6 files at r4, 2 of 2 files at r7, 1 of 2 files at r8.
Reviewable status: 2 of 6 files reviewed, 12 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

tests/CMakeLists.txt line 146 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

+ memcheck and pmemcheck

Done.

tests/common/stream_helpers.hpp line 23 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

looks like these classes are copy-pasted from example, please add info about that in the commit msg

Done.

tests/common/stream_helpers.hpp line 36 at r2 (raw file):

should it throw when there are no entries...?
I think it's the only way to fail using constructor. Other way is to provide fabric method.

tests/common/stream_helpers.hpp line 623 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

you can ASSERT here already, no need for returning bool from the func

I'd prefer to return bool and not use mix of throw and asserts for execution control.
I see some usage like preparing wrong timestamps, then validate > recovery > validate.

tests/common/stream_helpers.hpp line 629 at r2 (raw file):

you can ASSERT here already, no need for returning bool from the func
When gaps between timestamps are not allowed we expect exact values from 1 (PMEMSTREAM_FIRST_TIMESTAMP) to N, in other case we check only that timestamp(N) > timestamp(N-1).

tests/common/stream_helpers.hpp line 635 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

isn't it the same as entry_iterators.erase(oldest_iterator)?

You're right.

tests/unittest/timestamp.cpp line 69 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

you can use pmemstream_with_multi_empty_regions instead

Done.

tests/unittest/timestamp.cpp line 88 at r2 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

1

Done.

tests/unittest/timestamp.cpp line 104 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

pls make the CI green, e.g. by providing a proper styling

Done.

igchor

Reviewed 2 of 2 files at r11.
Reviewable status: all files reviewed (commit messages unreviewed), 16 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)

tests/common/stream_helpers.hpp line 627 at r10 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Done.

Not done?

tests/unittest/timestamp.cpp line 26 at r2 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Done.

why not use std::string as in other tests?

tests/unittest/timestamp.cpp line 48 at r10 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Done.

not done: const std::vector<>& regions

tests/unittest/timestamp.cpp line 117 at r10 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

*rc::gen::elementOf(regions))

Right, but:

I have some set of regions

Remove 1, then add 1

If something other will change (e.g. you miss another region, it will fail).
In your case you will miss it.

I don't understand what you mean by "miss another region". But nevermind, it's OK

lukaszstolarczuk

Reviewed 1 of 4 files at r12, all commit messages.
Reviewable status: 4 of 7 files reviewed, 16 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

-- commits line 6 at r13:
+ why?

tests/common/stream_helpers.hpp line 36 at r2 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

should it throw when there are no entries...?
I think it's the only way to fail using constructor. Other way is to provide fabric method.

what I'm saying is that a entry_iterator with no entries is a valid iterator - entries may come later on

tests/common/stream_helpers.hpp line 627 at r10 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

get_entries_from_regions + default value for 'regions' to get all entries in stream

hmm, maybe no default value (for consistency with other helpers), but please rename to _from_regions

tests/common/stream_helpers.hpp line 28 at r13 (raw file):

class entry_iterator {
 public:
	entry_iterator(pmemstream *stream, pmemstream_region &region) : stream(stream)

isn't this class taken (partially) from the example? please mention that in commit msg

tests/common/stream_helpers.hpp line 634 at r13 (raw file):

	/* Validation method to check timestamp order across regions when some entries/regions are missing */
	bool validate_timestamps_possible_gaps(const std::vector<struct pmemstream_region> &regions)

why do we need 2 functions instead of just 1?

tests/unittest/timestamp.cpp line 4 at r13 (raw file):

/* Copyright 2022, Intel Corporation */

#include "libpmemstream.h"

I think you rather want to include "unittest.hpp"

tests/unittest/timestamp.cpp line 36 at r13 (raw file):

	return run_test(test_config, [&] {
		return_check ret;
		ret += rc::check("verify increasing timestamp values in each region after synchronous append",

these are not "properties" - pls fix all tests' descriptions

tests/unittest/timestamp.cpp line 45 at r13 (raw file):

					 multithreaded_synchronous_append(stream, regions, data);

					 // Single region ordering validation

all // comments -> /**/

tests/unittest/timestamp.cpp line 84 at r13 (raw file):

				UT_ASSERT(stream.helpers.validate_timestamps_possible_gaps(regions));

				regions[pos] = stream.helpers.initialize_single_region(region_size, data);

you could use another set of data (extra_data) instead

tests/unittest/timestamp.cpp line 89 at r13 (raw file):

			});

		ret += rc::check(

if you're not adding this test case now, please remove this test and just leave a comment, what TC should be added in the future

igchor

Reviewable status: 4 of 7 files reviewed, 17 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

tests/common/rapidcheck_helpers.hpp line 144 at r13 (raw file):

template <>
struct Arbitrary<pmemstream_with_multi_empty_regions<true, true>> {

WHy do you implement a generator for specialized template? You can just implement it for all combinations:

template <bool I1, bool I2>
struct Arbitrary<pmemstream_with_multi_empty_regions<I1, I2>> {
	static Gen<pmemstream_with_multi_empty_regions<I1, I2>> arbitrary()
	{
		return gen::noShrink(gen::construct<pmemstream_with_multi_empty_regions<I1, I2>>(
			gen::arbitrary<pmemstream_test_base>(),
			gen::inRange<size_t>(1, TEST_DEFAULT_REGION_MULTI_MAX_COUNT)));
	}
};

KFilipek

Reviewed 2 of 3 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

-- commits line 6 at r13:

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

+ why?

Done.

tests/common/rapidcheck_helpers.hpp line 144 at r13 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

WHy do you implement a generator for specialized template? You can just implement it for all combinations:

template <bool I1, bool I2>
struct Arbitrary<pmemstream_with_multi_empty_regions<I1, I2>> {
	static Gen<pmemstream_with_multi_empty_regions<I1, I2>> arbitrary()
	{
		return gen::noShrink(gen::construct<pmemstream_with_multi_empty_regions<I1, I2>>(
			gen::arbitrary<pmemstream_test_base>(),
			gen::inRange<size_t>(1, TEST_DEFAULT_REGION_MULTI_MAX_COUNT)));
	}
};

Done.

tests/common/stream_helpers.hpp line 36 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

what I'm saying is that a entry_iterator with no entries is a valid iterator - entries may come later on

Right, but it's not the case.

tests/common/stream_helpers.hpp line 26 at r6 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Please remove the template, you don;t need it

Indeed xD

tests/common/stream_helpers.hpp line 18 at r10 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

do you need these?

Yes. I can link libpmemstream_internal.h, but then relative paths will fail in benchmark compilation. I lose 1 day to pożenić this and I failed, so I've reverted change and moved it here.

tests/common/stream_helpers.hpp line 627 at r10 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Not done?

Done.

tests/common/stream_helpers.hpp line 627 at r10 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

hmm, maybe no default value (for consistency with other helpers), but please rename to _from_regions

Done.

tests/common/stream_helpers.hpp line 28 at r13 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

isn't this class taken (partially) from the example? please mention that in commit msg

Done.

tests/common/stream_helpers.hpp line 634 at r13 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

why do we need 2 functions instead of just 1?

It's @igchor requirement to be more verbose on function calls.
validate_timestamps(regions, true) is less clear than validate_timestamps_possible_gaps(regions)

tests/unittest/timestamp.cpp line 26 at r2 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

why not use std::string as in other tests?

Now should be fine

tests/unittest/timestamp.cpp line 48 at r10 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

not done: const std::vector<>& regions

Done.

tests/unittest/timestamp.cpp line 4 at r13 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I think you rather want to include "unittest.hpp"

Done.

tests/unittest/timestamp.cpp line 36 at r13 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

these are not "properties" - pls fix all tests' descriptions

Done.

tests/unittest/timestamp.cpp line 45 at r13 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

all // comments -> /**/

Done.

tests/unittest/timestamp.cpp line 84 at r13 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

you could use another set of data (extra_data) instead

Done.

tests/unittest/timestamp.cpp line 89 at r13 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

if you're not adding this test case now, please remove this test and just leave a comment, what TC should be added in the future

Done.

KFilipek

Reviewable status: 4 of 7 files reviewed, 16 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

tests/common/stream_helpers.hpp line 23 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I think you can move this class into "pmem" namespace here

Done.

igchor

Reviewed 1 of 1 files at r16.
Reviewable status: 5 of 7 files reviewed, 14 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

tests/common/rapidcheck_helpers.hpp line 144 at r13 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Done.

Still not done? you dont need this specialization

KFilipek

Reviewed 1 of 3 files at r15, 1 of 1 files at r17, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)

tests/common/rapidcheck_helpers.hpp line 144 at r13 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Still not done? you dont need this specialization

Right, I missed it when committing some part. Sorry for that.

igchor

LGTM, but it does not close #211 until asynchronous cases are implemneted

Reviewed 1 of 1 files at r17.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @lukaszstolarczuk)

KFilipek · 2022-06-29T15:07:47Z

LGTM, but it does not close #211 until asynchronous cases are implemneted

Reviewed 1 of 1 files at r17.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @lukaszstolarczuk)

I've change description to not resolve issue #211

igchor

Reviewable status: 6 of 7 files reviewed, 13 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)

igchor

Reviewable status: 6 of 7 files reviewed, 14 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

tests/common/stream_helpers.hpp line 390 at r18 (raw file):

		struct pmemstream_entry entry;
		for (size_t i = 0; i < data.size(); ++i) {
			pmemstream_region_runtime *rrt = nullptr;

Hm, after this change, do we still need to set call_region_initialize in the generator at all? (do we need those extra template params to the generator?) We are not inserting anything concurrently now.

lukaszstolarczuk

Reviewed 1 of 3 files at r15, 1 of 1 files at r17, 1 of 1 files at r18, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @KFilipek)

-- commits line 7 at r18:
that also would require some explanation, I think, especially since there's no comment in the code for that part

tests/common/stream_helpers.hpp line 36 at r2 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Right, but it's not the case.

what's not the case? we may use this helper for other tests than just this one 😉

when you try to get_timestamp from the iterator or when using get_entries_from_region you have checks for valid iterators - so this check is not required on the creation time, I think

tests/common/stream_helpers.hpp line 18 at r10 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Yes. I can link libpmemstream_internal.h, but then relative paths will fail in benchmark compilation. I lose 1 day to pożenić this and I failed, so I've reverted change and moved it here.

when I asked this question it wasn't used anywhere 😉

tests/common/stream_helpers.hpp line 643 at r10 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Done.

you can drop Validation method from these 2 descriptions - it's rather obvious, these are methods 😉

tests/common/stream_helpers.hpp line 637 at r18 (raw file):

	}

	/* Validation method to check timestamp order across regions when some entries/regions are missing */

regions are missing may be misleading - I think you'll get an error (when creating an iterator) if you'll pass here a non-existing region...?
soo... I'd just leave here: ... when entries may be missing (it's not for sure that they are missing 😉 )

tests/common/stream_helpers.hpp line 637 at r18 (raw file):

	}

	/* Validation method to check timestamp order across regions when some entries/regions are missing */

timestamps' order

tests/common/stream_helpers.hpp line 675 at r18 (raw file):

				timestamps.back() == PMEMSTREAM_FIRST_TIMESTAMP + timestamps.size() - 1;
		}
		return true;

redundant return path

tests/unittest/timestamp.cpp line 45 at r13 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Done.

there are still some // comments in this PR

tests/unittest/timestamp.cpp line 56 at r18 (raw file):

				/* Multithreaded append to many regions with global ordering. */
				multithreaded_synchronous_append(stream, regions, data);

you can use the new helper func get_entries_from_regions(regions) to check if entries' count is as expected

This patch removes concurrent use of operator[] in append() helper function. If there is no region_runtime just passes nullptr to function. This approach avoid races that occur on concurrent appending to std::map.

class timestamp_iterator is taken from example 05_timestamp_based_order and slightly modified to just iterate through timestamps.

KFilipek

Reviewed all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lukaszstolarczuk)

-- commits line 7 at r18:

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

that also would require some explanation, I think, especially since there's no comment in the code for that part

Done.

tests/common/stream_helpers.hpp line 36 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

what's not the case? we may use this helper for other tests than just this one 😉

when you try to get_timestamp from the iterator or when using get_entries_from_region you have checks for valid iterators - so this check is not required on the creation time, I think

Done.

tests/common/stream_helpers.hpp line 643 at r10 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

you can drop Validation method from these 2 descriptions - it's rather obvious, these are methods 😉

Done.

tests/common/stream_helpers.hpp line 637 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

regions are missing may be misleading - I think you'll get an error (when creating an iterator) if you'll pass here a non-existing region...?

soo... I'd just leave here: ... when entries may be missing (it's not for sure that they are missing 😉 )

Done.

tests/common/stream_helpers.hpp line 637 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

timestamps' order

Done.

tests/common/stream_helpers.hpp line 675 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

redundant return path

Done.

tests/unittest/timestamp.cpp line 45 at r13 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

there are still some // comments in this PR

Done.

tests/unittest/timestamp.cpp line 56 at r18 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

you can use the new helper func get_entries_from_regions(regions) to check if entries' count is as expected

Done.

igchor

Reviewed 1 of 4 files at r19.
Reviewable status: 4 of 7 files reviewed, 8 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)

lukaszstolarczuk

Reviewed 4 of 4 files at r19, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @KFilipek)

KFilipek requested a review from lukaszstolarczuk June 17, 2022 13:08

lukaszstolarczuk reviewed Jun 17, 2022

View reviewed changes

igchor reviewed Jun 20, 2022

View reviewed changes

KFilipek force-pushed the extra_timestamp_tests branch 3 times, most recently from 4c33786 to 49400dc Compare June 21, 2022 11:58

igchor reviewed Jun 21, 2022

View reviewed changes

KFilipek force-pushed the extra_timestamp_tests branch 13 times, most recently from 6379437 to 0a08233 Compare June 22, 2022 10:41

igchor reviewed Jun 22, 2022

View reviewed changes

KFilipek force-pushed the extra_timestamp_tests branch 8 times, most recently from 70c129e to 1ae3a0d Compare June 22, 2022 16:32

KFilipek commented Jun 23, 2022

View reviewed changes

KFilipek force-pushed the extra_timestamp_tests branch 2 times, most recently from f6b7048 to b28547d Compare June 27, 2022 11:36

igchor suggested changes Jun 27, 2022

View reviewed changes

KFilipek force-pushed the extra_timestamp_tests branch 3 times, most recently from 0e2d400 to 7404ee1 Compare June 27, 2022 13:14

lukaszstolarczuk reviewed Jun 27, 2022

View reviewed changes

igchor suggested changes Jun 27, 2022

View reviewed changes

KFilipek force-pushed the extra_timestamp_tests branch 2 times, most recently from babc1d8 to 889b638 Compare June 29, 2022 13:07

KFilipek commented Jun 29, 2022

View reviewed changes

KFilipek force-pushed the extra_timestamp_tests branch from 889b638 to 208a198 Compare June 29, 2022 13:11

KFilipek commented Jun 29, 2022

View reviewed changes

KFilipek marked this pull request as ready for review June 29, 2022 13:12

igchor suggested changes Jun 29, 2022

View reviewed changes

KFilipek force-pushed the extra_timestamp_tests branch from 208a198 to 3ab12ab Compare June 29, 2022 14:43

KFilipek commented Jun 29, 2022

View reviewed changes

igchor reviewed Jun 29, 2022

View reviewed changes

KFilipek force-pushed the extra_timestamp_tests branch from 3ab12ab to 7390c2f Compare June 29, 2022 15:02

igchor approved these changes Jun 30, 2022

View reviewed changes

igchor reviewed Jun 30, 2022

View reviewed changes

lukaszstolarczuk suggested changes Jun 30, 2022

View reviewed changes

KFilipek added 2 commits June 30, 2022 13:14

Fix concurrent append to std::map

fb52d09

This patch removes concurrent use of operator[] in append() helper function. If there is no region_runtime just passes nullptr to function. This approach avoid races that occur on concurrent appending to std::map.

[tests] check timestamps order after multithreaded synchronous append

912cbfa

class timestamp_iterator is taken from example 05_timestamp_based_order and slightly modified to just iterate through timestamps.

KFilipek force-pushed the extra_timestamp_tests branch from 7390c2f to 912cbfa Compare June 30, 2022 13:44

KFilipek commented Jun 30, 2022

View reviewed changes

igchor reviewed Jun 30, 2022

View reviewed changes

lukaszstolarczuk approved these changes Jun 30, 2022

View reviewed changes

lukaszstolarczuk merged commit 6dac589 into pmem:master Jun 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extra timestamp tests #217

Extra timestamp tests #217

KFilipek commented Jun 17, 2022 •

edited

Loading

lukaszstolarczuk left a comment

igchor left a comment

igchor left a comment

igchor left a comment

KFilipek left a comment

igchor left a comment

lukaszstolarczuk left a comment

igchor left a comment

KFilipek left a comment

KFilipek left a comment

igchor left a comment

KFilipek left a comment

igchor left a comment

KFilipek commented Jun 29, 2022

igchor left a comment

igchor left a comment

lukaszstolarczuk left a comment

KFilipek left a comment

igchor left a comment

lukaszstolarczuk left a comment

Extra timestamp tests #217

Extra timestamp tests #217

Conversation

KFilipek commented Jun 17, 2022 • edited Loading

TODO:

Description from issue:

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

KFilipek left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

KFilipek left a comment

Choose a reason for hiding this comment

KFilipek left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

KFilipek left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

KFilipek commented Jun 29, 2022

igchor left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

KFilipek left a comment

Choose a reason for hiding this comment

igchor left a comment

Choose a reason for hiding this comment

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

KFilipek commented Jun 17, 2022 •

edited

Loading