Opened 2 years ago

Last modified 2 years ago

#22196 assigned enhancement

Configure descriptor sources using method chaining

Reported by: karsten Owned by: metrics-team
Priority: Medium Milestone:
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The two descriptor sources DescriptorReader and DescriptorParser are instantiated by DescriptorSourceFactory, configured, and then executed. This requires state checks and leads to quite verbose application code. An exception to this is DescriptorCollector which is state-less but which has a method with 5 parameters, which also does not produce very readable code.

We should consider switching to configuring descriptor sources using method chanining. Here's a code sample from a few months ago, written for another ticket.

iwakeh rightly commented on that other ticket: "configuration of a descriptor source by fluent-style (or method chaining) is fine, but metrics-lib currently has the DescriptorSourceFactory approach, which would need to be adapted. That is, I see two things: the ideas around the code of DescriptorSourceBuilder are ideas about a new way of configuration and not exception/error handling in metrics-lib, i.e. a different ticket (there is one for redesign of the interface hierarchy, I'll look for it). Second, the current configuration and descriptor source instanciation need to be considered together (in that other ticket)."

Not assigning to a milestone on purpose. This likely needs more thoughts before we can write code and make plans for releasing that.

Child Tickets

Change History (7)

comment:1 Changed 2 years ago by iwakeh

Milestone: metrics-lib 2.0.0

comment:2 Changed 2 years ago by karsten

Milestone: metrics-lib 2.0.0metrics-lib 1.9.0

Let's try to squeeze this into 1.9.0, so that we can finish all interface changes before 2.0.0 when we can remove deprecated interface parts and after which we won't be able to do that again before 3.0.0.

comment:3 Changed 2 years ago by karsten

I had the following idea when working on #22141:

Maybe we can avoid configuring descriptor sources entirely and go with overloaded, state-less methods like the one we have in DescriptorCollector. The main obstacle here is DescriptorReader's parse history where the application needs a way to save the parse history to file to disk once it's done processing descriptors. Maybe we can work around that obstacle by using minLastModified in DescriptorReader as we're using in DescriptorCollector. Here are possible method signatures in the three non-deprecated descriptor sources:

DescriptorParser:
  Iterable<Descriptor> parse(byte[] rawDescriptorBytes);

DescriptorCollector:
  void collect(
      File localDirectory,
      String collecTorBaseUrl,
      String... remoteDirectories);
  void collect(
      long minLastModified,
      File localDirectory,
      String collecTorBaseUrl,
      String... remoteDirectories);
  void collect(
      boolean deleteExtraneousLocalFiles,
      long minLastModified,
      File localDirectory,
      String collecTorBaseUrl,
      String... remoteDirectories);

DescriptorReader:
  Iterable<Descriptor> read(
      File... descriptorFiles);
  Iterable<Descriptor> read(
      long minLastModified,
      File... descriptorFiles);
  Iterable<Descriptor> read(
      int maxDescriptorsInQueue,
      long minLastModified,
      File... descriptorFiles);

An application that wants to process only newly added descriptors would start by retrieving a current timestamp to mark the beginning of its current execution. It would then load the last execution timestamp from disk (rather than let metrics-lib load the last parse history) and pass that timestamp as minLastModified to both collect() and read(). In fact, it might want to subtract 15 or 30 minutes from that timestamp to account for clock skew with the CollecTor server. And when it's done processing descriptors it saves the current timestamp for the next execution (rather than let metrics-lib save the parse history to disk).

comment:4 Changed 2 years ago by iwakeh

Yes, using overloading here makes sense (and not using the configuration methods safes the hassle of determining when configuration is allowed or not).

comment:5 Changed 2 years ago by karsten

Owner: changed from metrics-team to karsten
Status: newassigned

Great! I'll tackle this one after #22141 then. Thanks for checking!

comment:6 Changed 2 years ago by karsten

Milestone: metrics-lib 1.9.0

As explained in this comment, we can't just dump the parse history and replace it with a minLastModified timestamp. We should reconsider which of these changes we can make, and we won't be able to do that for 1.9.0 anymore. Moving out of the currently planned milestones.

comment:7 Changed 2 years ago by karsten

Owner: changed from karsten to metrics-team

Handing over to metrics-team, because I'm not currently working on this.

Note: See TracTickets for help on using tickets.