JEP-207 - External Build Log Storage - 1 or 2 classes in Jenkins

[Oleg Nenashev]

Hi all,

I would like to detach the discussion of one of the topics in JEP-207. One of the design concerns from Jesse Glick was Why LoggingMethod and LogBrowser are separated?. I have created 2 separate abstract classes for Log reporting and Browsing, because it looked natural from the External Logging PoV (logs are reported to a single log sink which somehow propagates them to a storage which is a separate implementation). Jesse's concern was that such abstraction layer should be probably moved to External API Plugin instead of having that in the core directly.

I have spent some time to implement that, and here are the patches:

• https://github.com/oleg-nenashev/jenkins/pull/8
• https://github.com/jenkinsci/external-logging-api-plugin/pull/2
• https://github.com/jenkinsci/external-logging-elasticsearch-plugin/pull/2

So, it didn't take long to update the code to support that. It would be great to get feedback about the implementation:

Pros:

• Jenkins core becomes more simple (~10% decrease in the implementation size)
• API and extension points became more simple, there are just few new classes added
• More glue code was moved to External Logging API
• Nothing really changed for the reference External Logging Elasticsearch reference implementation. There were only some renamings
  

Cons:

• IMHO LogStorage class in the core is a bit full of various methods. LogBrowser was already a bit full thanks to hudson.model.Run APIs, but by merge we produce a mega-class with a dozen of abstract methods
• Some methods could have had default implementations in the core, but I decided to move them to External Logging API so we can update them if needed (performance of default implementations is pretty bad).
  
• ExternalLogStorage implementation now does call routing to 2 ExternalLogSomething implementations and extension points.
  
• The calls are not THAT frequent, so there is no performance concern there
• BUT: if we introduce new methods in the core at some point, we will have to implement default implementations in the core
  

Generally I lean towards agreeing with Jesse that the new implementation is a bit better as long External Logging API hides the complexity. Would appreciate extra feedback.

Thanks in advance,

Oleg

[Jesse Glick]

On Mon, Aug 6, 2018 at 9:57 AM Oleg Nenashev <o.v.ne...@gmail.com> wrote:
> Jesse's concern was that such abstraction layer should be probably moved to External API Plugin instead of having that in the core directly.

FTR my advice was that this division should not be mandated by the SPI
at all (including in the API plugin)—that at the basic level, an
implementation of external logging should be in control of both
sending and displaying log messages however it sees fit (since the
details might be tightly coupled); and that insofar as there may be
particular cases where it makes sense to offer the administrator
independent choices for logging and display (for example, because
there is a nontrivial Fluentd logging implementation which could be
coupled with various storage backends), that we can always introduce a
more specific SPI for that technology stack.

Oleg’s preference was to retain a separation, if necessary introducing
a filtering system which would allow plugins to express whether a
particular combination of extensions made any sense. My request was to
at least keep this design in a plugin to make it easier to revisit the
decision later if we need to.

> Some methods could have had default implementations in the core, but I decided to move them to External Logging API so we can update them if needed

Makes sense to me.

[Oleg Nenashev]

Yeah, I have shortened the background and likely messed up the context a bit.

Thanks for the clarification!

BR, Oleg

--
You received this message because you are subscribed to the Google Groups "Jenkins Cloud Native SIG" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkins-cloud-native-sig+unsub...@googlegroups.com.
To post to this group, send email to jenkins-cloud-native-sig@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkins-cloud-native-sig/CANfRfr3eC212mg1AFgt72SjEHg4aXeayysJtoPLGB21S86xAig%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Oleg Nenashev]

Submitted the JEP patch: https://github.com/jenkinsci/jep/pull/169

This patch addresses this topic and also other comments fro Jesse in the original discussion