consider removal of broken objects #730
Conversation
ocaml/src/maintenance.ml
Outdated
|
|
||
| Lwt_log.debug_f | ||
| "Found buckets %s for namespace_id:%Li" | ||
| ([%show : (Policy.policy * int64) list] bucket_count) | ||
| namespace_id >>= fun () -> | ||
|
|
||
| mgr_access # get_namespace_by_id ~namespace_id |
There was a problem hiding this comment.
This will query the albamgr each time, we may want to avoid that?
There was a problem hiding this comment.
that's silly and easy to avoid as we have the name a level above. will do.
ocaml/src/maintenance.ml
Outdated
| let prefix_len = String.length prefix in | ||
| len >= prefix_len | ||
| && | ||
| String.sub namespace_name 0 prefix_len = prefix |
There was a problem hiding this comment.
We should probably have a String.starts_with helper function in prelude, which avoids the string copy done here. I have the feeling we do this elsewhere in the code too, but I can't find it / recall where...
There was a problem hiding this comment.
I had that feeling too, looked for it, couldn't find it. Will add to prelude.
ocaml/src/maintenance_helper.ml
Outdated
| end | ||
| end | ||
|
|
||
| let neg_compare_buckets b1 b2 = |
There was a problem hiding this comment.
maybe remove this function, and let it immediately sort in the desired way in compare_buckets above?
ocaml/src/maintenance_helper.ml
Outdated
| let compare_bucket_safety | ||
| (k1, _, fragment_count1, _) | ||
| (k2, _, fragment_count2, _) = | ||
| (fragment_count2 - k2) - (fragment_count1 - k1) |
There was a problem hiding this comment.
you flipped this around which is a bit counter intuitive?
ocaml/src/maintenance_test.ml
Outdated
| let expected =[ | ||
| ( 1, 2, 3, 1), Rewrite; (* safety: 2 *) | ||
| (16, 8, 20, 3), Regenerate; (* safety: 4 *) | ||
| (16, 8, 24, 6), Rebalance; |
There was a problem hiding this comment.
can you add 1 extra Rewrite & Regenerate case so that part of the sorting is fully tested too?
| @@ -101,6 +101,16 @@ end | |||
| module String = struct | |||
| include String | |||
|
|
|||
| let starts_with s prefix = | |||
There was a problem hiding this comment.
This implementation is inefficient due to bounds checks, and it can throw Invalid_argument 'index out of bounds' when the prefix is longer than the string s.
Probably the easiest solution is to use Core.String.is_prefix ~prefix s
don't throw if string shorter than prefix use unsafe_get
No description provided.