Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix 1-phase of testing #100

Merged
merged 1 commit into from
Jul 24, 2024
Merged

fix 1-phase of testing #100

merged 1 commit into from
Jul 24, 2024

Conversation

osalyk
Copy link
Contributor

@osalyk osalyk commented Jul 11, 2024

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @osalyk)


CMakeLists.txt line 34 at r1 (raw file):

# pmdk_tests - main CMakeLists.txt

cmake_minimum_required(VERSION 3.5)

It seems you have two commits changing this particular line. Why just not squash them together?


functions.cmake line 53 at r1 (raw file):

function(download_gtest)
	include(ExternalProject)
	set(GTEST_VERSION 1.12.0)

Please comment on why did you pick this particular version instead of the newest one in the commit.


README.md line 13 at r1 (raw file):

To build pmdk-tests, the following packages are required:
* **[PMDK](https://github.com/pmem/pmdk)**
* **CMake - version 3.0 or greater**
  1. It is incorrect.
  2. Changing this line seems very tied to commits changing the actual cmake_minimum_required() line. Why is this change separate?

README.md line 73 at r1 (raw file):

### Other Requirements ###
Python scripts in pmdk-tests are compatible with Python 3.4.
Install libboost-filesystem library.

Are you sure about the filesystem part? libboost is a vast set of libraries. You might had a different one in mind.


src/tests/ras/utils/test_phase/local_test_phase.cc line 46 at r1 (raw file):

  int i = 0;
  boost::algorithm::copy_if(config_.begin(), config_.end(),

It is a little odd. The normal way of functions live is rather boost->std rather than std->boost.

Do you know what has happened in this particular case?
Maybe you can past here the error code and we can build the case together.

Code quote:

boost::algorithm

Copy link
Contributor Author

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @janekmi)


CMakeLists.txt line 34 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It seems you have two commits changing this particular line. Why just not squash them together?

Done.


functions.cmake line 53 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please comment on why did you pick this particular version instead of the newest one in the commit.

Done.


README.md line 13 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…
  1. It is incorrect.
  2. Changing this line seems very tied to commits changing the actual cmake_minimum_required() line. Why is this change separate?

Done.


README.md line 73 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Are you sure about the filesystem part? libboost is a vast set of libraries. You might had a different one in mind.

Experimentally verified.


src/tests/ras/utils/test_phase/local_test_phase.cc line 46 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It is a little odd. The normal way of functions live is rather boost->std rather than std->boost.

Do you know what has happened in this particular case?
Maybe you can past here the error code and we can build the case together.

copy_if is a C++11 addition.

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @osalyk)


CMakeLists.txt line 39 at r3 (raw file):

# ignore all the warnings
add_definitions(-w)

Is it necessary? What warnings bother us?

Code quote:

# ignore all the warnings
add_definitions(-w)

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 40 at r3 (raw file):

}

/* Change status of shutdown state */

Suggestion:

Change the status of the shutdown state

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 41 at r3 (raw file):

/* Change status of shutdown state */
void set_sds_func(bool state) {

Note: It configures the PMEMOBJ's behaviour ONLY when a pool is created.

Suggestion:

set_sds_at_create_func

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 44 at r3 (raw file):

	int ret = pmemobj_ctl_set(NULL, "sds.at_create", &state);
	if (ret) {
		std::cerr << "Failed to set sds: " << pmemobj_errormsg() << std::endl;

Won't kill us and at the same time is a notch more informative.

Suggestion:

Failed to set sds.at_create

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 168 at r3 (raw file):

TEST_P(MovePoolClean, TC_MOVE_POOL_CLEAN_phase_1) {
  /* Step1 */
  set_sds_func(false);

It should be done conditionally only when the underlying device does not support SDS.

Code quote:

  set_sds_func(false);

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 189 at r3 (raw file):

  /* Step5 */
  set_sds_func(true);

It does not work on pmemobj_open().

Copy link
Contributor Author

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @janekmi)


CMakeLists.txt line 39 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Is it necessary? What warnings bother us?

Among other things, a warning about a too old version of cmake.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 40 at r3 (raw file):

}

/* Change status of shutdown state */

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 41 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Note: It configures the PMEMOBJ's behaviour ONLY when a pool is created.

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 189 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It does not work on pmemobj_open().

Done.

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk)


CMakeLists.txt line 39 at r3 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

Among other things, a warning about a too old version of cmake.

IMHO it is unnecessary. Note it won't affect the dependencies' builds so you end up with warnings anyway.

I recommend removing it.

@osalyk osalyk changed the title Update packages fix 1-phase of testing Jul 22, 2024
Copy link
Contributor Author

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 10 files reviewed, 2 unresolved discussions (waiting on @janekmi)


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 168 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It should be done conditionally only when the underlying device does not support SDS.

Done.


CMakeLists.txt line 39 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

IMHO it is unnecessary. Note it won't affect the dependencies' builds so you end up with warnings anyway.

I recommend removing it.

Done.

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 7 files at r5, all commit messages.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @osalyk)


-- commits line 2 at r5:

Suggestion:

fix the 1st-phase of testing

src/tests/ras/unsafe_shutdown/local_move_tests.h line 43 at r5 (raw file):

  std::string dest_pool_dir;
  bool enough_dimms;
  bool pmem;

Suggestion:

bool src_pool_dir_is_pmem;

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 40 at r5 (raw file):

}

bool pmem;

Probably unnecessary. Please see the SetUp() below and the UnsafeShutdown class.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 59 at r5 (raw file):

      tc.enough_dimms = false;
    }
    tc.pmem = true;

Suggestion:

    move_param tc;
    tc.description = "from unsafely shutdown DIMM to non-pmem";
    if (unsafe_dn.size() >= 1) {
      tc.enough_dimms = true;
      tc.src_pool_dir = unsafe_dn[0].GetTestDir();
      tc.src_pool_dir_is_pmem = true;
      tc.dest_pool_dir = test_phase.GetTestDir();
    } else {
      tc.enough_dimms = false;
    }

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 74 at r5 (raw file):

      tc.enough_dimms = false;
    }
    tc.pmem = false;

Please apply the formatting proposed above.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 89 at r5 (raw file):

      tc.enough_dimms = false;
    }
    tc.pmem = true;

.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 104 at r5 (raw file):

      tc.enough_dimms = false;
    }
    tc.pmem = true;

.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 119 at r5 (raw file):

      tc.enough_dimms = false;
    }
    tc.pmem = true;

.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 134 at r5 (raw file):

      tc.enough_dimms = false;
    }
    tc.pmem = false;

.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 147 at r5 (raw file):

  src_pool_path_ = param.src_pool_dir + GetNormalizedTestName() + "_pool";
  dest_pool_path_ = param.dest_pool_dir + GetNormalizedTestName() + "_pool";
  pmem = param.pmem;

Suggestion:

create_on_pmem = param.src_pool_dir_is_pmem;
UnsafeShutdown::SetUp()

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 156 at r5 (raw file):

 * Trigger unsafe shutdown after closing the pool.
 * \test
 *          \li \c Step1. Disable SDS. Create pool on device. Enable SDS. / SUCCESS

Suggestion:

\li \c Step1. Create a pool on the device. / SUCCESS

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 169 at r5 (raw file):

  /* Step1 */
  if (pmem == false)
    set_sds_at_create_func(false);

Covered by the SetUp method.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 175 at r5 (raw file):

                               << pmemobj_errormsg();
  if (pmem == false)
    set_sds_at_create_func(true);

Covered by the TearDown method.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 211 at r5 (raw file):

      << "Insufficient number of DIMMs to run this test";
  src_pool_path_ = param.src_pool_dir + GetNormalizedTestName() + "_pool";
  dest_pool_path_ = param.dest_pool_dir + GetNormalizedTestName() + "_pool";

Suggestion:

dest_pool_path_ = param.dest_pool_dir + GetNormalizedTestName() + "_pool";
create_on_pmem = params.src_pool_dir_is_pmem;
UnsafeShutdown::SetUp()

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 219 at r5 (raw file):

 * can be opened.
 * \test
 *          \li \c Step1. Disable SDS. Create pool on device. Enable SDS. / SUCCESS

Suggestion:

\li \c Step1. Create a pool on the device. / SUCCESS

src/tests/ras/unsafe_shutdown/local_move_tests.cc line 233 at r5 (raw file):

  /* Step1 */
  if (pmem == false)
    set_sds_at_create_func(false);

.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 238 at r5 (raw file):

                               << pmemobj_errormsg();
  if (pmem == false)
    set_sds_at_create_func(true);

.


src/tests/ras/unsafe_shutdown/local_replicas_tests.h line 43 at r5 (raw file):

  bool enough_dimms;
  bool is_syncable;
  bool pmem;

? Please see the discussion below.

Suggestion:

bool is_pmem;

src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 40 at r5 (raw file):

}

bool is_pmem;

Replaced by the UnsafeShutdown::create_on_pmem field.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 46 at r5 (raw file):

  ASSERT_TRUE(param.enough_dimms)
      << "Insufficient number of DIMMs to run this test";
  is_pmem = param.pmem;

? Please see the discussion below.

Suggestion:

UnsafeShutdown::SetUp();

src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 78 at r5 (raw file):

  if (is_pmem == false)
    set_sds_at_create_func(true);

Covered by the SetUp() method.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 84 at r5 (raw file):

      << pmemobj_errormsg();
  if (is_pmem == false)
    set_sds_at_create_func(false);

Covered by the TeardDown() method.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 159 at r5 (raw file):

      tc.enough_dimms = false;
    }
    tc.pmem = true;

Is it just me or all the cases in this file should be actually be pmem == true. So, there is no need to move these values around and probably even calling the base class constructor is unnecessary UnsafeShutdown::SetUp();.

Code quote:

    tc.pmem = true;

src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 297 at r5 (raw file):

   * partially on two us-dimms.
   * XXX - Because of the change about always-on SDS,
   * this test case does not make sense anymore. */

Please apply accordingly where necessary.

Suggestion:

   * Note: The use case was deemed irrelevant and deleted. */

src/tests/ras/unsafe_shutdown/unsafe_shutdown.h line 59 at r5 (raw file):

  bool PassedOnPreviousPhase() const;
  std::string GetNormalizedTestName() const;
  int PmempoolRepair(std::string pool_file_path) const;

Suggestion:

  int PmempoolRepair(std::string pool_file_path) const;

  void SetUp() override;
  void TearDown() override;

src/tests/ras/unsafe_shutdown/unsafe_shutdown.h line 71 at r5 (raw file):

 protected:
  bool close_pools_at_end_ = true;

Suggestion:

 protected:
  bool close_pools_at_end_ = true;
  bool create_on_pmem = true;

src/tests/ras/unsafe_shutdown/unsafe_shutdown.h line 83 at r5 (raw file):

};

void set_sds_at_create_func(bool state);

IMHO the most natural way is to integrate this as part of the

Suggestion:

 private:
  const ::testing::TestInfo& GetTestInfo() const {
    return *::testing::UnitTest::GetInstance()->current_test_info();
  }
  std::string GetPassedStamp() const {
    return test_phase_.GetTestDir() + GetNormalizedTestName() + "_passed";
  }
  void StampPassedResult() const;
  void SetSdsAtCreate() const;
};

src/tests/ras/unsafe_shutdown/unsafe_shutdown.cc line 89 at r5 (raw file):

  return pmempool_check_end(ppc);
}

Code snippet:

void UnsafeShutdown::SetUp() {
  if (!create_on_pmem) {
  	SetSdsAtCreate(false)
  }
}

void UnsafeShutdown::TearDown() {
  if (!create_on_pmem) {
  	SetSdsAtCreate(true)
  }
}

src/tests/ras/unsafe_shutdown/unsafe_shutdown.cc line 91 at r5 (raw file):

/* Change the status of the shutdown state */
void set_sds_at_create_func(bool state) {

Suggestion:

void UnsafeShutdown::SetSdsAtCreate(bool state) {

Copy link
Contributor Author

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @janekmi)


src/tests/ras/unsafe_shutdown/local_move_tests.h line 43 at r5 (raw file):

  std::string dest_pool_dir;
  bool enough_dimms;
  bool pmem;

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 40 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Probably unnecessary. Please see the SetUp() below and the UnsafeShutdown class.

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 59 at r5 (raw file):

      tc.enough_dimms = false;
    }
    tc.pmem = true;

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 74 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please apply the formatting proposed above.

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 89 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 104 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 119 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 134 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 147 at r5 (raw file):

  src_pool_path_ = param.src_pool_dir + GetNormalizedTestName() + "_pool";
  dest_pool_path_ = param.dest_pool_dir + GetNormalizedTestName() + "_pool";
  pmem = param.pmem;

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 156 at r5 (raw file):

 * Trigger unsafe shutdown after closing the pool.
 * \test
 *          \li \c Step1. Disable SDS. Create pool on device. Enable SDS. / SUCCESS

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 169 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Covered by the SetUp method.

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 175 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Covered by the TearDown method.

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 211 at r5 (raw file):

      << "Insufficient number of DIMMs to run this test";
  src_pool_path_ = param.src_pool_dir + GetNormalizedTestName() + "_pool";
  dest_pool_path_ = param.dest_pool_dir + GetNormalizedTestName() + "_pool";

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 219 at r5 (raw file):

 * can be opened.
 * \test
 *          \li \c Step1. Disable SDS. Create pool on device. Enable SDS. / SUCCESS

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 233 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.


src/tests/ras/unsafe_shutdown/local_move_tests.cc line 238 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.

Done.


src/tests/ras/unsafe_shutdown/local_replicas_tests.h line 43 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

? Please see the discussion below.

Done.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 40 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Replaced by the UnsafeShutdown::create_on_pmem field.

Done.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 46 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

? Please see the discussion below.

Done.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 78 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Covered by the SetUp() method.

Done.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 84 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Covered by the TeardDown() method.

Done.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 159 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Is it just me or all the cases in this file should be actually be pmem == true. So, there is no need to move these values around and probably even calling the base class constructor is unnecessary UnsafeShutdown::SetUp();.

Done.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 297 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please apply accordingly where necessary.

Done.


src/tests/ras/unsafe_shutdown/unsafe_shutdown.h line 59 at r5 (raw file):

  bool PassedOnPreviousPhase() const;
  std::string GetNormalizedTestName() const;
  int PmempoolRepair(std::string pool_file_path) const;

Done.


src/tests/ras/unsafe_shutdown/unsafe_shutdown.h line 71 at r5 (raw file):

 protected:
  bool close_pools_at_end_ = true;

Done.


src/tests/ras/unsafe_shutdown/unsafe_shutdown.h line 83 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

IMHO the most natural way is to integrate this as part of the

Done.


src/tests/ras/unsafe_shutdown/unsafe_shutdown.cc line 89 at r5 (raw file):

  return pmempool_check_end(ppc);
}

Done.


src/tests/ras/unsafe_shutdown/unsafe_shutdown.cc line 91 at r5 (raw file):

/* Change the status of the shutdown state */
void set_sds_at_create_func(bool state) {

Done.

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk)


src/tests/ras/unsafe_shutdown/local_replicas_tests.h line 43 at r6 (raw file):

  bool enough_dimms;
  bool is_syncable;
  bool is_pmem;

It seems this is not being used in the latest iteration. I believe we can just delete it.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 44 at r6 (raw file):

  ASSERT_TRUE(param.enough_dimms)
      << "Insufficient number of DIMMs to run this test";
  UnsafeShutdown::SetUp();

Unnecessary.

Copy link
Contributor Author

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)


src/tests/ras/unsafe_shutdown/local_replicas_tests.h line 43 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It seems this is not being used in the latest iteration. I believe we can just delete it.

Done.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 44 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Unnecessary.

Done.

Copy link

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 5 files at r3, 1 of 7 files at r5, 5 of 6 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

Copy link

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

Copy link

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @janekmi and @osalyk)


src/tests/ras/unsafe_shutdown/unsafe_shutdown.h line 63 at r7 (raw file):

  void SetUp() override;
  void TearDown() override;

TearDown is obsolete

Suggestion:

  void SetUp() override;

src/tests/ras/unsafe_shutdown/unsafe_shutdown.cc line 108 at r7 (raw file):

    SetSdsAtCreate(true);
  }
}

TearDown is obsolete

Suggestion:

void UnsafeShutdown::SetUp() {
  if (!create_on_pmem) {
    SetSdsAtCreate(false);
  }
  else {
    SdsAtCreate(true);
  }
}

Copy link
Contributor Author

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 10 files reviewed, 4 unresolved discussions (waiting on @grom72 and @janekmi)


src/tests/ras/unsafe_shutdown/unsafe_shutdown.h line 63 at r7 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

TearDown is obsolete

Done.


src/tests/ras/unsafe_shutdown/unsafe_shutdown.cc line 108 at r7 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

TearDown is obsolete

Done.

Copy link

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 6 files at r6, 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk)


src/tests/ras/unsafe_shutdown/local_replicas_tests.h line 43 at r8 (raw file):

  bool enough_dimms;
  bool is_syncable;
  bool src_pool_dir_is_pmem;

I believe this variable is redundant. It is always true.
Please see SyncLocalReplica::SetUp().


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 44 at r8 (raw file):

  ASSERT_TRUE(param.enough_dimms)
      << "Insufficient number of DIMMs to run this test";
}

Suggestion:

void SyncLocalReplica::SetUp() {
  sync_local_replica_tc param = GetParam();
  ASSERT_TRUE(param.enough_dimms)
      << "Insufficient number of DIMMs to run this test";
  create_on_pmem = true;
  UnsafeShutdown::SetUp();
}

Copy link
Contributor Author

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @grom72 and @janekmi)


src/tests/ras/unsafe_shutdown/local_replicas_tests.h line 43 at r8 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I believe this variable is redundant. It is always true.
Please see SyncLocalReplica::SetUp().

Done.


src/tests/ras/unsafe_shutdown/local_replicas_tests.cc line 44 at r8 (raw file):

  ASSERT_TRUE(param.enough_dimms)
      << "Insufficient number of DIMMs to run this test";
}

Done.

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

Copy link

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

Copy link

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

Copy link

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

@janekmi janekmi merged commit b636a12 into master Jul 24, 2024
2 checks passed
@osalyk osalyk deleted the add_sds branch July 25, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants