DAOS-6632 obj: add support for hints on the oclass generate API #4831

mchaarawi · 2021-03-02T21:27:07Z

choose oclass based on set redundancy and redundance factor of container.
adjust oclass if shard sizing is indicated in hint, otherwise use
max sharding.

Signed-off-by: Mohamad Chaarawi mohamad.chaarawi@intel.com

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2021-03-03T01:13:45Z

Test stage Scan CentOS 7 RPMs completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4831/1/execution/node/1161/log

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2021-03-05T10:50:48Z

Test stage Scan CentOS 7 RPMs completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4831/2/execution/node/1150/log

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1

Style warning(s) for job https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-4831/4/
Please review https://wiki.hpdd.intel.com/display/DC/Coding+Rules

daosbuild1 · 2021-03-09T00:36:55Z

src/tests/suite/daos_obj.c

+oclass_auto_setting(void **state)
+{
+	test_arg_t		*arg = *state;
+	uuid_t		 	uuid;


(style) please, no space before tabs

daosbuild1 · 2021-03-09T00:36:55Z

src/tests/suite/daos_obj.c

+	assert_rc_equal(rc, 0);
+
+	feat_array = DAOS_OF_DKEY_UINT64 | DAOS_OF_KV_FLAT | DAOS_OF_ARRAY;
+	feat_byte_array = DAOS_OF_DKEY_UINT64 | DAOS_OF_KV_FLAT | DAOS_OF_ARRAY_BYTE;


(style) line over 80 characters

daosbuild1 · 2021-03-09T00:36:55Z

src/tests/suite/daos_obj.c

+	feat_byte_array = DAOS_OF_DKEY_UINT64 | DAOS_OF_KV_FLAT | DAOS_OF_ARRAY_BYTE;
+	feat_kv = DAOS_OF_KV_FLAT;
+
+        prop = daos_prop_alloc(1);


(style) code indent should use tabs where possible

daosbuild1 · 2021-03-09T00:36:56Z

src/tests/suite/daos_obj.c

+
+	print_message("OID settings with container RF0:\n");
+	/** create container with rf = 0 */
+        prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;


(style) code indent should use tabs where possible

daosbuild1 · 2021-03-09T00:36:56Z

src/tests/suite/daos_obj.c

+	print_message("OID settings with container RF0:\n");
+	/** create container with rf = 0 */
+        prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;
+        prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF0;


(style) code indent should use tabs where possible

daosbuild1 · 2021-03-09T00:36:56Z

src/tests/suite/daos_obj.c

+oclass_auto_setting(void **state)
+{
+	test_arg_t		*arg = *state;
+	uuid_t		 	uuid;


Suggested change

uuid_t uuid;

uuid_t uuid;

daosbuild1 · 2021-03-09T00:36:56Z

src/tests/suite/daos_obj.c

+	feat_byte_array = DAOS_OF_DKEY_UINT64 | DAOS_OF_KV_FLAT | DAOS_OF_ARRAY_BYTE;
+	feat_kv = DAOS_OF_KV_FLAT;
+
+        prop = daos_prop_alloc(1);


Suggested change

prop = daos_prop_alloc(1);

prop = daos_prop_alloc(1);

daosbuild1 · 2021-03-09T00:36:56Z

src/tests/suite/daos_obj.c

+        prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;
+        prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF0;


Suggested change

prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;

prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF0;

prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;

prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF0;

daosbuild1 · 2021-03-09T00:36:57Z

src/tests/suite/daos_obj.c

+        prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;
+        prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF1;


Suggested change

prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;

prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF1;

prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;

prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF1;

daosbuild1 · 2021-03-09T00:36:57Z

src/tests/suite/daos_obj.c

+        prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;
+        prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF2;


Suggested change

prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;

prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF2;

prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;

prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF2;

daosbuild1 · 2021-03-09T00:37:02Z

Test stage checkpatch completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4831/4/execution/node/72/log

Updated patch

daosbuild1 · 2021-03-09T00:42:30Z

src/tests/suite/daos_obj.c

+	if (res == DAOS_RES_REPL) {
+		assert_int_equal(attr->u.rp.r_num, nr);
+	} else if (res == DAOS_RES_EC) {
+		assert_int_equal(attr->u.ec.e_p, nr-1);


Suggested change

assert_int_equal(attr->u.ec.e_p, nr-1);

assert_int_equal(attr->u.ec.e_p, nr - 1);

daosbuild1 · 2021-03-09T00:42:33Z

Test stage checkpatch completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4831/5/execution/node/73/log

Updated patch

daosbuild1 · 2021-03-09T00:45:27Z

src/tests/suite/daos_obj.c

+	if (res == DAOS_RES_REPL) {
+		assert_int_equal(attr->u.rp.r_num, nr);
+	} else if (res == DAOS_RES_EC) {
+		assert_int_equal(attr->u.ec.e_p, nr-1);


Suggested change

assert_int_equal(attr->u.ec.e_p, nr-1);

assert_int_equal(attr->u.ec.e_p, nr - 1);

daosbuild1 · 2021-03-09T00:45:31Z

Test stage checkpatch completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4831/6/execution/node/72/log

Updated patch

daosbuild1

LGTM. No errors found by checkpatch.

- choose oclass based on set redundancy and redundance factor of container. - adjust oclass if shard sizing is indicated in hint, otherwise use max sharding. Signed-off-by: Mohamad Chaarawi <mohamad.chaarawi@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

gnailzenh · 2021-03-11T08:50:39Z

src/include/daos_obj.h

+#define DAOS_OC_RDD_MAX_VAL	((1ULL << DAOS_OC_RDD_BITS) - 1)
+#define DAOS_OC_SHD_MAX_VAL	((1ULL << DAOS_OC_SHD_BITS) - 1)
+#define DAOS_OC_RDD_MASK	(DAOS_OC_RDD_MAX_VAL << DAOS_OC_RDD_SHIFT)
+#define DAOS_OC_SHD_MASK	(DAOS_OC_SHD_MAX_VAL << DAOS_OC_SHD_SHIFT)


I think these bits are for "hints" of the API correct? Then I'd suggest to use a different prefix because DAOS_OC has been used by "class", how about change it to DAOS_OCH_

ok then we would need to make this change to the actual hints that we also merged to 1.2 as this would be an API change now..

gnailzenh · 2021-03-11T08:59:43Z

src/object/obj_class.c

+		return -DER_NOMEM;
+
+	prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;
+	rc = daos_cont_query(coh, NULL, prop, NULL);


I think we could potentially get this on container open (it does retrieve some properties), and save the RF in "dc_cont". container query is an expensive operation, if user calls this function for millions of times in a loop, then it might have problem.

i think this might already be retrieved. thanks for pointing that out. will fix/=

- rename hints to add OCH prefix Signed-off-by: Mohamad Chaarawi <mohamad.chaarawi@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1

LGTM. No errors found by checkpatch.

mchaarawi · 2021-03-18T22:13:13Z

@wangdi1 @liuxuezhao @gnailzenh @johannlombardi
could you guys please review? thanks!

liuxuezhao · 2021-03-23T14:28:01Z

src/object/obj_class.c

+	switch (rf_factor) {
+	case DAOS_PROP_CO_REDUN_RF0:
+		if (rdd == DAOS_OCH_RDD_RP)
+			cid = OC_RP_2GX;


RF0 means has no data protection right? so should be OC_SX for DAOS_OCH_RDD_RP, and seems is invalid parameter if with RF0 and rdd == DAOS_OCH_RDD_EC?

RF0 means no data protection as the minimum, but one can still do data protection, right?
this gets set when cont prop is RF0, and user says he wants replication in the hint. i think this should be valid?
if the hint is 0, it gets set to SX.
why is rdd = DAOS_OCH_RDD_EC invalid here?

ok, this one looks fine to me.

liuxuezhao · 2021-03-23T14:32:40Z

src/object/obj_class.c

+		if (rdd == DAOS_OCH_RDD_RP)
+			cid = OC_RP_2GX;
+		else if (rdd == DAOS_OCH_RDD_EC || ofeats & DAOS_OF_ARRAY ||
+			 ofeats & DAOS_OF_ARRAY_BYTE)


just confirm that does it mean that dc_array obj always map to EC ?

by default, yes if the container has RF > 0

liuxuezhao · 2021-03-23T14:34:37Z

src/object/obj_class.c

+		else if (rdd == DAOS_OCH_RDD_EC || ofeats & DAOS_OF_ARRAY ||
+			 ofeats & DAOS_OF_ARRAY_BYTE)
+			/** TODO - this should be GX when supported */
+			cid = OC_EC_2P2G1;


must it be 2P1 or 2P2? is it possible to select 4P2, 8P2, 16P2 in future?

yea, i got those from @wangdi1
there are some that are not available today, and this is what was suggested as a reasonable default if not set by user.

oh, I mean P2 is the most popular. But I thought 4P2G1 might be most post popular choice generally. But I thought(heard) ANL might mostly use 8P2 or 16P2 for performance reason. Hmm, maybe domain_nr should be involved for consideration?
if domain_nr >=10
cid = 8P2G1
else if domain_rn >= 6
cid = 4P2G1
else
cid = 2P2G1.

agreed that we probably need more correlation between the domain but also the target nr to set also the GX.
we can tackle this in a future PR when we add more EC object class?

I've added all classes (commit 806d1cf), I assume we are not going to land it to 1.2, so we can switch to new classes in follow-on patch

we actually want to add to 1.2.
let me see what you added and adjust this PR i guess.

ah wait.. i thought you were talking about my PR. but if we are not landing your PR to 1.2, then i can't change it here since we want to land this to 1.2.
i can do the change to master in a future PR to master. so if you are OK with this, please land it

liuxuezhao · 2021-03-23T14:36:23Z

src/object/obj_class.c

+		break;
+	case DAOS_PROP_CO_REDUN_RF3:
+	case DAOS_PROP_CO_REDUN_RF4:
+		return -DER_INVAL;


RF3/RF4 is not supported temporarily, and may be supported later?

it is supported at the container level, but there is no oclass to support it today.. we need to add the oclass for it.
by default setting, this will not work today unless we create oclass to support it.

gnailzenh · 2021-03-25T12:20:34Z

src/object/obj_class.c

+		else if (rdd == DAOS_OCH_RDD_EC || ofeats & DAOS_OF_ARRAY ||
+			 ofeats & DAOS_OF_ARRAY_BYTE)
+			/** TODO - this should be GX when supported */
+			cid = OC_EC_2P2G1;


I've added all classes (commit 806d1cf), I assume we are not going to land it to 1.2, so we can switch to new classes in follow-on patch

- choose oclass based on set redundancy and redundance factor of container. - adjust oclass if shard sizing is indicated in hint, otherwise use max sharding. Signed-off-by: Mohamad Chaarawi <mohamad.chaarawi@intel.com>

… (#5181) - choose oclass based on set redundancy and redundance factor of container. - adjust oclass if shard sizing is indicated in hint, otherwise use max sharding. Signed-off-by: Mohamad Chaarawi <mohamad.chaarawi@intel.com>

daosbuild1 reviewed Mar 2, 2021

View reviewed changes

mchaarawi force-pushed the mschaara/oclass_hints branch from afd0187 to b5e2b50 Compare March 5, 2021 01:38

daosbuild1 reviewed Mar 5, 2021

View reviewed changes

mchaarawi force-pushed the mschaara/oclass_hints branch from b5e2b50 to 8f0466b Compare March 8, 2021 14:09

daosbuild1 reviewed Mar 8, 2021

View reviewed changes

mchaarawi force-pushed the mschaara/oclass_hints branch from 8f0466b to b53789e Compare March 9, 2021 00:35

daosbuild1 previously requested changes Mar 9, 2021

View reviewed changes

mchaarawi force-pushed the mschaara/oclass_hints branch from b53789e to 30bcd35 Compare March 9, 2021 00:40

daosbuild1 previously requested changes Mar 9, 2021

View reviewed changes

mchaarawi force-pushed the mschaara/oclass_hints branch from 30bcd35 to c4c635d Compare March 9, 2021 14:11

daosbuild1 reviewed Mar 9, 2021

View reviewed changes

DAOS-6632 obj: add support for hints on the oclass generate API

6a0bd40

- choose oclass based on set redundancy and redundance factor of container. - adjust oclass if shard sizing is indicated in hint, otherwise use max sharding. Signed-off-by: Mohamad Chaarawi <mohamad.chaarawi@intel.com>

mchaarawi force-pushed the mschaara/oclass_hints branch from c4c635d to 6a0bd40 Compare March 9, 2021 21:35

daosbuild1 reviewed Mar 9, 2021

View reviewed changes

mchaarawi requested review from liuxuezhao, gnailzenh, johannlombardi and wangdi1 March 9, 2021 23:31

gnailzenh reviewed Mar 11, 2021

View reviewed changes

- don't query for redundancy factory as it's already fetched on open

b3db4a5

- rename hints to add OCH prefix Signed-off-by: Mohamad Chaarawi <mohamad.chaarawi@intel.com>

daosbuild1 reviewed Mar 11, 2021

View reviewed changes

mchaarawi requested a review from gnailzenh March 12, 2021 13:45

Merge branch 'master' into mschaara/oclass_hints

a3acd3b

daosbuild1 reviewed Mar 12, 2021

View reviewed changes

daosbuild1 reviewed Mar 14, 2021

View reviewed changes

Merge branch 'master' into mschaara/oclass_hints

31d40c7

daosbuild1 reviewed Mar 15, 2021

View reviewed changes

Merge branch 'master' into mschaara/oclass_hints

1c6d5a7

daosbuild1 reviewed Mar 16, 2021

View reviewed changes

liuxuezhao reviewed Mar 23, 2021

View reviewed changes

liuxuezhao approved these changes Mar 23, 2021

View reviewed changes

wangdi1 approved these changes Mar 23, 2021

View reviewed changes

mchaarawi requested a review from a team March 24, 2021 14:06

gnailzenh approved these changes Mar 25, 2021

View reviewed changes

johannlombardi merged commit fba8d98 into master Mar 25, 2021

johannlombardi deleted the mschaara/oclass_hints branch March 25, 2021 17:59

ashleypittman mentioned this pull request Apr 28, 2021

1.2 dfuse single #5582

Closed

ashleypittman mentioned this pull request May 20, 2021

1.2 proc #5753

Closed

		prop->dpp_entries[0].dpe_type = DAOS_PROP_CO_REDUN_FAC;
		prop->dpp_entries[0].dpe_val = DAOS_PROP_CO_REDUN_RF0;

	assert_int_equal(attr->u.ec.e_p, nr-1);
	assert_int_equal(attr->u.ec.e_p, nr - 1);

DAOS-6632 obj: add support for hints on the oclass generate API #4831

DAOS-6632 obj: add support for hints on the oclass generate API #4831

Conversation

mchaarawi commented Mar 2, 2021

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Mar 3, 2021

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Mar 5, 2021

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Mar 9, 2021

Choose a reason for hiding this comment

daosbuild1 commented Mar 9, 2021

Choose a reason for hiding this comment

daosbuild1 commented Mar 9, 2021

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnailzenh Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

mchaarawi commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnailzenh Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchaarawi Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnailzenh Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

gnailzenh Mar 11, 2021 •

edited

Loading

gnailzenh Mar 25, 2021 •

edited

Loading

mchaarawi Mar 25, 2021 •

edited

Loading

gnailzenh Mar 25, 2021 •

edited

Loading