Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

m0crate kv(index) test fixes. #461

Merged
merged 17 commits into from
Mar 18, 2021
Merged

Conversation

somnathbghule
Copy link
Contributor

@somnathbghule somnathbghule commented Feb 24, 2021

There are improvement areas such as,

Correctness in output logs (for all log levels) of m0crate KV test run.
	a. Following parameters are not being used by m0crate kv test but still displayed in the output logs.
		Before Fix:
			info: average size:          65536
			info: maximal size:          1048576
			info: block size:            0
		After Fix:
			No need to display this if KV test run is running.
	b. Following parameter shows wrong output.
		Before Fix:
			info: number of operations:  1000 (OP_COUT: 32 was passed. always shows the same value.). 
		After Fix:
			info: number of operations:  32
	c. If we read following output it looks like percentage of ops but its actually 
            count of number of operations remaining. 
		Before Fix:
			trace: dix: prcnt: [5, 4, 0, 0]		
		After Fix: 
			trace: dix: ops remaining: [5, 4, 0, 0]
	d.  Result should shows details about the test run,
            Before Fix:
                            result: all, 0.133912
            After Fix:
                           result: Total, 0.133912 s
	                          Avg time per ops.
	                          PUT, <time taken by put ops> s
	                          GET, <time taken by get ops> s and so on

Implementation of specifying the specific KEY and VALUE sizes in kv test run, sizes can be number (int) or random.

    key_size=sizeof(struct m0_fid)
    value_size=RECORD_SIZE-key_size;
    here RECORD_SIZE is tunable parameter available in .yaml file.
    There is no way/option available to set specific size of key and value.

@welcome
Copy link

welcome bot commented Feb 24, 2021

Thanks for your contribution in opening this pull request! Now you can be rewarded with a CORTX sticker by requesting cortx sticker
In the meantime, please check out the contributing guidelines and explore other ways you can get involved.

@cla-bot
Copy link

cla-bot bot commented Feb 24, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Somnath Ghule.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cortx-admin
Copy link

Can one of the admins verify this patch?

@somnathbghule somnathbghule changed the title [IN-PROGRESS] m0crate kv test fixes. [IN-PROGRESS] m0crate kv(index) test fixes. Feb 25, 2021
@cla-bot
Copy link

cla-bot bot commented Feb 25, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Somnath Ghule.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

4 similar comments
@cla-bot
Copy link

cla-bot bot commented Feb 25, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Somnath Ghule.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Feb 26, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Somnath Ghule.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Feb 26, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Somnath Ghule.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Feb 26, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Somnath Ghule.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@somnathbghule somnathbghule marked this pull request as ready for review February 26, 2021 14:35
@somnathbghule somnathbghule changed the title [IN-PROGRESS] m0crate kv(index) test fixes. m0crate kv(index) test fixes. Feb 26, 2021
@somnathbghule
Copy link
Contributor Author

Result after fix:

info: done workload 0
info: ---------------------------------------
info: starting workload 0
info: workload type index/3
info: random seed: 0
info: number of threads: 1
info: dix: End of operations.
result: total_s, 0.231018, avg_time_per_op_ns, 7452178, key_size_bytes, 1024, value_size_bytes, 32768, ops, 31
result: put_s, 0.118350, avg_time_per_op_ns, 6228926, ops, 19
result: get_s, 0.044113, avg_time_per_op_ns, 5514173, ops, 8
result: del_s, 0.004043, avg_time_per_op_ns, 4043037, ops, 1
result: next_s, 0.026017, avg_time_per_op_ns, 8672168, ops, 3
Total: time=[0:231017544] ([0:007452178] per op) ops=31
PUT: [0:118349598] ([0:006228926] per op) ops=19
GET: [0:044113390] ([0:005514173] per op) ops=8
DEL: [0:004043037] ([0:004043037] per op) ops=1
NEXT: [0:026016505] ([0:008672168] per op) ops=3
info: done workload 0
info: ---------------------------------------

@cla-bot
Copy link

cla-bot bot commented Mar 1, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Somnath Ghule, root.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Mar 1, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Somnath Ghule.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Mar 1, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Somnath Ghule.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Mar 1, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Somnath Ghule.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Mar 1, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: somnathbghule.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@@ -308,6 +308,8 @@ struct cr_idx_w {
struct cr_time_measure_ctx exec_time_ctx;
size_t exec_time;
enum cr_op_selector op_selector;
m0_time_t kv_op_acc_time[CRATE_OP_NR];

Choose a reason for hiding this comment

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

how about name exec_time_per_kv_op_type?

Add comment
// see enum cr_opcode, 4 kv op types for now

Copy link

@kaustubh-d kaustubh-d Mar 1, 2021

Choose a reason for hiding this comment

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

you may want to have struct cr_idx_w_results and move measurements outside workload descriptor.

example

struct cr_idx_ops_result {
      const char * c_op_type; // label
      int                 op_type; // comes from cr_opcode::CRATE_OP_PUT
      m0_time_t    total_time_all_ops_s;
      m0_time_t    time_per_op_ns;
}

struct cr_idx_w_results {
     m0_time_t                  total_time; // entire workload
     cr_idx_ops_result      ops_result[CRATE_OP_NR]; // per op result for each put, get, del, next
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as per discussion.

@@ -308,6 +308,8 @@ struct cr_idx_w {
struct cr_time_measure_ctx exec_time_ctx;
size_t exec_time;
enum cr_op_selector op_selector;
m0_time_t kv_op_acc_time[CRATE_OP_NR];
int kv_op_count[CRATE_OP_NR];

Choose a reason for hiding this comment

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

total_ops_count_per_kv_op_type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

w.wit->key_size, w.wit->value_size, w.nr_ops_total);

if (w.kv_op_count[CRATE_OP_PUT])
fprintf(stdout, "result: put_s, %f, avg_time_per_op_ns, %"PRIu64", ops, %d\n",

Choose a reason for hiding this comment

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

ops should not have _s for secs, like put_s

use key as total_time_s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

t->elapsed, m0_time_nanoseconds(t->test_time) / w.nr_ops_total,
w.wit->key_size, w.wit->value_size, w.nr_ops_total);

if (w.kv_op_count[CRATE_OP_PUT])

Choose a reason for hiding this comment

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

Simply to reduce fprintf/if-else ladder as discussed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@stale
Copy link

stale bot commented Mar 15, 2021

This issue/pull request has been marked as needs attention as it has been left pending without new activity for 4 days. Tagging @huanghua78 @mukundkanekar for appropriate assignment. Sorry for the delay & Thank you for contributing to CORTX. We will get back to you as soon as possible.

WORKLOAD_SEED: tstamp
NUM_KVP: 8
NXRECORDS: default # int or default
RECORD_SIZE: 32 # int [units] or random

Choose a reason for hiding this comment

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

since we are updating these files, lets update to use latest params as key size and value size and just deprecate RECORD_SIZE

Choose a reason for hiding this comment

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

same for other sample yaml files.

Choose a reason for hiding this comment

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

yes, we can remove the "RECORD_SIZE" from these sample yaml config files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed RECORD_SIZE and MAX_RSIZE from sample yaml config files.

somnathbghule and others added 17 commits March 17, 2021 05:33
Removed unwanted info from kv test run o/p, added KEY_SIZE support.

Added m0crate index value_size changes.

Cleaned record size changes and added changes for max key and value.

added changes for total op time and time per ops in ns.

added changes for total op time and time per ops in ns.

Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
@huanghua78
Copy link

ok to test

@huanghua78 huanghua78 merged commit 7a734e7 into Seagate:main Mar 18, 2021
@welcome
Copy link

welcome bot commented Mar 18, 2021

Thanks for your contribution to CORTX! 🎉

@kaustubh-d
Copy link

Thanks @huanghua78 for your support in getting this reviewed and merged! Thanks @somnathbghule for the improvements!

andriytk pushed a commit to andriytk/cortx-motr that referenced this pull request Apr 1, 2021
* Removed unwanted info from kv test run o/p, added KEY_SIZE support.
* Added m0crate index value_size changes.
* Cleaned record size changes and added changes for max key and value.
* Added changes for total op time and time per ops in ns.
* Removed RECORD_SIZE and MAX_RSIZE from tests/*.yaml files.

Signed-off-by: somnathbghule <somnath.b.ghule@seagate.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants