chore: Final pass addressing any outstanding comments #87
Conversation
keelerm84
force-pushed
the
mk/sc-243772/expand-contract-tests
branch
from
8c09081
to
a728012
Compare
| @@ -641,7 +641,7 @@ impl Client { | |||
| context: &Context, | |||
| flag_key: &str, | |||
| default_stage: Stage, | |||
| ) -> (Stage, MigrationOpTracker) { | |||
| ) -> (Stage, Arc<Mutex<MigrationOpTracker>>) { | |||
There was a problem hiding this comment.
I mentioned this on the docs PR, but should we change this to return the Arc<Mutex<>>? Both the kit and the blocks would require doing this at some point anyway so it's kind of a convenience in a sense.
You will also see in this PR I removed the internal mutex in the tracker as well. No need to pay for two lock penalties.
| match params.operation { | ||
| launchdarkly_server_sdk::Operation::Read => { | ||
| let result = migrator | ||
| .read( | ||
| ¶ms.context, |
There was a problem hiding this comment.
We don't need to consume the context, and the parameter order change makes this consistent with the variation methods.
| @@ -131,21 +131,17 @@ impl Serialize for MigrationOpEvent { | |||
| key: self.key.clone(), | |||
| value: self.evaluation.value, | |||
| default: self.default_stage, | |||
| // QUESTION: In the ruby implementation, this can be nil. Why not here? | |||
There was a problem hiding this comment.
The ruby one is needlessly checking for nil. 🤦🏼
There was a problem hiding this comment.
In client-side SDKs the reason is optional, so maybe it is for bootstrapping from ruby? But it would always be populated.
| reason: self.evaluation.reason.clone(), | ||
| variation_index: self.evaluation.variation_index, | ||
| version: self.version, | ||
| }; | ||
| state.serialize_field("evaluation", &evaluation)?; | ||
|
|
||
| // TODO: Add sampling here if it is set and not 1 |
There was a problem hiding this comment.
Done above. Forgot to remove this then.
| let mut measurements = vec![]; | ||
| if !self.invoked.is_empty() { | ||
| measurements.push(MigrationOpMeasurement::Invoked(&self.invoked)); | ||
| } | ||
|
|
||
| // TODO: There is something here to do with consistency check ratio |
| @@ -43,79 +43,63 @@ pub struct MigrationWriteResult<T> { | |||
| // provided results are equal, this method will return true and false otherwise. | |||
| type MigrationComparisonFn<T> = fn(&T, &T) -> bool; | |||
|
|
|||
| struct MigrationConfig<T, FO, FN> | |||
| struct MigrationConfig<P, T, FO, FN> | |||
There was a problem hiding this comment.
Originally this was requiring the read or write payload parameter to be the same type as the return of those methods. That isn't a requirement of the spec so we should reflect that appropriately. So lots of type changes here.
| { | ||
| /// Create a new migrator builder instance with the provided client. | ||
| pub fn new(client: Arc<Client>) -> Self { | ||
| MigratorBuilder { | ||
| client, | ||
| read_execution_order: ExecutionOrder::Parallel, | ||
| read_execution_order: ExecutionOrder::Concurrent, |
There was a problem hiding this comment.
Also changed this to reflect the actual execution method.
| @@ -24,8 +23,6 @@ pub struct MigrationOpTracker { | |||
| context: Context, | |||
| detail: Detail<Stage>, | |||
| default_stage: Stage, | |||
|
|
|||
| mutex: Mutex<()>, | |||
There was a problem hiding this comment.
Removing the interior mutex as mentioned previously.
keelerm84
force-pushed
the
mk/sc-247332/polish
branch
from
a2c9652
to
15bf189
Compare
No description provided.