-
Notifications
You must be signed in to change notification settings - Fork 37
Conversation
40d2591
to
2c1e5f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 11 of 11 files at r1, 4 of 4 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @johnsonw)
iml-api/src/graphql/mod.rs, line 1211 at r2 (raw file):
false => true, }) .collect();
Collecting in between iterator calls is an antipattern. Directly append further iterator calls instead (xs
below should go here, with one collect
in the end).
iml-services/iml-device/src/lib.rs, line 66 at r2 (raw file):
pub async fn create_target_cache(pool: &PgPool) -> Result<Vec<Target>, ImlDeviceError> { let xs: Vec<Target> = sqlx::query!("select state, name, active_host_id, host_ids, filesystems, uuid, mount_path, dev_path, fs_type::text from target")
SELECT state, name, active_host_id, host_ids, filesystems, uuid, mount_path, dev_path, fs_type::text FROM target
iml-services/iml-device/src/lib.rs, line 79 at r2 (raw file):
dev_path: x.dev_path, fs_type: x.fs_type.unwrap_or_else(|| "ldiskfs".to_string()).into(), }
Maybe implement impl From<TargetRecord> for TargetBuilder
and do it like this: TargetBuilder::from(x).with_fs_type(x.fs_type.unwrap_or_else(|| "ldiskfs".to_string()).into()).build()
?
iml-wire-types/src/lib.rs, line 1917 at r2 (raw file):
} impl From<String> for FsType {
I think caller should just do &s
instead of providing an impl for all types that can coerce to string slice.
8ddd171
to
23c7a2a
Compare
Just wanted to share my testing results. I setup a clean cluster with an ldiskfs monitored filesystem. The table was populated as expected with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above is being used when parsing a line from the ldev conf. A line could represent a zfs target, such as:
zeno1 zeno5 zeno-OST0000 zfs:lustre-zeno1/ost1
or an ldiskfs target:
oss1.local oss2.local fs2-OST0000 ldiskfs:/mnt/ost2-0
Unless there is another valid format that I'm not aware of, it should always have either ldiskfs
or zfs
. But since I'm converting from an &str
to an FsType
, I need to account for all other strings. If it isn't either ldiskfs
or zfs
, how should we handle it? Should I create a third variant, Unknown
?
I think returning an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 9 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ip1981, @jgrund, and @johnsonw)
I just tried rebuilding and it passed this time. Should be ready. |
There was a problem hiding this 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, 6 unresolved discussions (waiting on @ip1981, @jgrund, and @johnsonw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 11 files at r1, 2 of 4 files at r2, 4 of 9 files at r3, 1 of 1 files at r4, 5 of 5 files at r5.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ip1981, @jgrund, and @johnsonw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 4 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ip1981, @jgrund, and @johnsonw)
There was a problem hiding this 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 r7.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ip1981, @jgrund, and @johnsonw)
8b10a9e
to
46a26cf
Compare
The fs_type data should be included in the targets table. This will be used when creating the ldev conf and likely other things as well. Signed-off-by: johnsonw <wjohnson@whamcloud.com>
Signed-off-by: johnsonw <wjohnson@whamcloud.com>
… lower case. Signed-off-by: johnsonw <wjohnson@whamcloud.com>
45d4cd6
to
df6e8ba
Compare
More updates:
|
Signed-off-by: johnsonw <wjohnson@whamcloud.com>
Signed-off-by: johnsonw <wjohnson@whamcloud.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 9 files at r8, 4 of 4 files at r9.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ip1981, @jgrund, @johnsonw, and @utopiabound)
The fs_type data should be included in the targets table. This will be
used when creating the ldev conf and likely other things as well.
Signed-off-by: johnsonw wjohnson@whamcloud.com
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)