liurenjie1024 commented on PR #135:
URL: https://github.com/apache/iceberg-rust/pull/135#issuecomment-1894895024
> > This design is kind of too complicated for me. I still prefer the
approach we used in icelake, and don't overuse too much traits for just saving
code.
>
> Actually, this design is to solve a problem I meet in icelake now.🤔 In
icelake, we use an interface like the following:
>
> ```
> impl FanoutPartitionWriter {
> fn metrics(&self) -> PartitionMetrics
> }
> ```
>
> And we have a corresponding monitor writer:
>
> ```
> struct ParititionMetricsMonitorWriter {
> writer: FanoutPartitionWriter
> }
> ```
>
> But after I introduced a new partition writer: `PrecomputePartitionWriter`
, I found that I couldn't reuse the `ParititionMetricsMonitorWriter` because
it's coupled with `FanoutPartitionWriter`. **But the metrics(state) they expose
are the same**, so I think maybe we need to abstract the interface that expose
the metrics(state).
These kinds of refactoring helps avoiding some code duplication, but I don't
think this is good abstraction. Good abstraction should be clean and easy to
understand. Even your example is not quite convincible, for example, what if
the metrics of these two partition writers diverge someday in future, e.g. I
want to count the latency of computing these partition calculation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
