Refactor/effeciencies #69
Conversation
| private def genPost: Gen[Method.Post] = Gen.oneOf(genApi(0), genApi(200)).map(Method.Post) | ||
| private def genGet: Gen[Method.Get] = Gen.oneOf(fixedApis, genApi(0), genApi(1)).map(Method.Get) | ||
| private def genHead: Gen[Method.Head] = Gen.oneOf(fixedApis, genApi(0), genApi(1)).map(Method.Head) | ||
| private def genPost(pathFreq: PathFrequencies): Gen[Method.Post] = |
There was a problem hiding this comment.
We can probably extract a template method here to avoid duplication and simplify testing, but that's a not a big deal.
|
|
||
| def fixedApis: Gen[Api] = Gen.oneOf(GenI, GenIce) | ||
|
|
||
| def genApi(nEvents: Int): Gen[Api] = | ||
| (nEvents match { | ||
| def genApi(apiType: Int): Gen[Api] = |
There was a problem hiding this comment.
Finally makes sense. Previously we had: HttpRequest#genPost (genApi(200)).
| st.map(_._3).map(buildRequesst).evalMap(req => client.status(req)).void) | ||
| st | ||
| .map(_._3) | ||
| .map(_.getOrElse(throw new RuntimeException("Http sink received no data"))) |
There was a problem hiding this comment.
I wonder if we need to raise here.
Doing something along the lines of seems a bit more idiomatic:
st
.map(_._3.toList) // we filter None out here
.flatMap(Stream.emits)
.map(buildRequesst)
.evalMap(req => client.status(req)).voidWe already have the logic for logging event count after it's done and completed.
There was a problem hiding this comment.
I dropped in a runtime exception because we to date have no sensible configuration handling of what formats go with what options - we just make assumptions about what the user wants to do. I'd like to change that, but until we do, this is just a janky stopgap - at least the app crashes when we configure the wrong format for the wrong target.
We already have the logic for logging event count after it's done and completed.
I don't think we do - by my reading of the code (which isn't very reliable), for the other formats we are counting that before passing to the sink. And it also appears not to be reliably accurate (but that's for another day). I couldn't see any logic in the app where the sink passes a result back up to the main flow of the app - I think we should do that for http target results at least - but it was beyond my abilities to get that done before I 'ran out of road' on this so to speak.
There was a problem hiding this comment.
I should add I'm not sure if I addressed the comment, I'm not sure I understand it 100% but we can discuss!
| @@ -63,7 +66,7 @@ object Main extends IOApp { | |||
|
|
|||
| } | |||
|
|
|||
| type GenOutput = (collector.CollectorPayload, List[Event], HttpRequest) | |||
| type GenOutput = (Option[collector.CollectorPayload], Option[List[Event]], Option[HttpRequest]) | |||
There was a problem hiding this comment.
Do I understand correctly, that Option[List[Event]] is about communicating that the _._2 here is only appearing under particular condition? None is functionally equivalent to List.empty
There was a problem hiding this comment.
Previously, we generated all 3 formats every time. This PR changes it to only generate the format we are interested in according to the configuration.
Option[List[Event]] is about defining the type for something which is None if not configured, but is List[Event] if configured to be so.
| @@ -155,13 +173,15 @@ object Main extends IOApp { | |||
| def fileSink(fileConfig: Config.Output.File): Pipe[F, GenOutput, Unit] = | |||
| RotatingSink.rotate(config.payloadsPerFile) { idx => | |||
| val pipe1: Pipe[F, GenOutput, Nothing] = | |||
| _.map(_._1) | |||
| _.map(_._1.getOrElse(throw new RuntimeException("File sink receieved no collector payload data"))) | |||
There was a problem hiding this comment.
Same question as with other sinks + RuntimeException applies.
| @@ -242,7 +262,7 @@ object Main extends IOApp { | |||
|
|
|||
| eventStream | |||
| .take(config.payloadsTotal.toLong) | |||
| .zipWithScan1((0, 0)) { case (idx, el) => (el._2.length + idx._1, 1 + idx._2) } | |||
| .zipWithScan1((0, 0)) { case (idx, el) => (el._2.getOrElse(Seq()).length + idx._1, 1 + idx._2) } | |||
There was a problem hiding this comment.
.map(_.length).getOrElse(0) maybe? That'd avoid allocating memory for seq/empty array. Means the same though.
| // In fact there's likely a simpler pattern for this whole chunk o logic | ||
| // I think ideally the config might be refactored to make this simpler | ||
| ( | ||
| collPayloadNeeded match { |
There was a problem hiding this comment.
Option.when(collPayloadNeeded)(runGen(collPayload, rng))May be what you're looking for
| // If no duplicate config is provided, profide a configuration that produces no duplicates. | ||
| val dups = config.duplicates.getOrElse(Config.Duplicates(0,0,0,0)) | ||
| Stream.repeatEval( | ||
| Sync[F].delay( { |
There was a problem hiding this comment.
| Sync[F].delay( { | |
| Sync[F].delay { |
colmsnowplow
force-pushed
the
release/0.6.0
branch
from
4cde200 to
2e39a8a
Compare
This PR refactors two things:
There are further improvments that can be found, such as refactoring config and allowing more combinations of sink & type - but this is as much as I got done on the flight back from the team offsite :)