Move sincedb logic into separate class

[andsel]

Release notes

[rn:skip]

What does this PR do?

Move the logic for storing and loading SinceDB in its own entity, separating unassigned from assigned concepts.
This changes aim to create non mutating instances and eliminating conditional paths when managing the load, store and update of the since DB data.

Why is it important/What is the impact to the user?

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• [ ] I have made corresponding changes to the documentation
• [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
• [ ] I have added tests that prove my fix is effective or that my feature works

Author's Checklist

• [ ]

How to test this PR locally

Related issues

[robbavey]

I'm not sure about this approach - it threw me for a loop when I was trying to figure out how an "assigned" SinceDB object came to be created.

At first glance, this looks like a mutation method on the SinceDB object, rather than a method that constructs a new object, and requires the caller to use this newly created version of the object.
Calling updatePosition without using the return value would not get caught and could introduce some subtle bugs when expanding the use of updatePosition outside of the close method.

[andsel]

You a right, it was a mutation method, then I switched SinceDB to be an immutable class value object without changing it. I'll provide a better name.

[andsel]

Renamed to retrieveNewPosition.

[robbavey]

Is there a reason why you moved from the Optional usage to the Null object pattern? The Null Object pattern here introduces some complexity, including the use of a tricky inheritance, which requires constructing the superclass with "bogus" parameters, such as Paths.get(System.getProperty("user.home"), and requires this method that "looks" like a mutation?

[andsel]

The motivation was the using the Optional<SinceDB> as a field, forced me to check if the value is present to execute some actions or not. For example, with optional the sinceDb.flush() has to be surrounded with presence check:

finally {
  if (sinceDb.isPresent() {
    sinceDb.get().flush();
  }
}

becomes more straightforward with the Null parttern

finally {
  sinceDb.flush();
}

Reading through the Optional javadoc it's intended to be used as method return type where there is the concept of "no result".
Having a field of a class Optional<> , when we can't use the ifPresent and map transformation methods, for to have a lot of optional_instance.get() calls.

So with the Null pattern we moved the if-present logic from ifs to method dispatch, and think provide more context knowledge then the Optional.

[andsel]

I also thought that having the SinceDB as non mutating class could help us when in the plugin we have concurrent update of it, and this should happen when we save the sinceDB from a scheduler thread.

[robbavey]

I wonder if it would make sense to have a SinceDB interface with static methods to retrieve a loaded sinceDB and to "update" an existing sinceDB, with concrete implementations of AssignedDB and UnassignedDB. This would remove the need to populate a constructor with "fake" values, and would remove the need to have an instance method on SinceDB, which requires the user of the class to know that the class is intended to replace the existing instance:

Something along the lines of

interface SinceDB {

  Path getPath();
  void flush();
  Path getCurrentSegment();
  long getOffset();
  boolean isAssigned();

  /**
   * Load SinceDB from given path
   * @param Path of sinceDB
   * @return new instance of SinceDB. This instance may be unassigned if the path does not exist, or has no entries written 
   *
   */
  static SinceDB fromPath(Path sinceDbPath) throws IOException;

  /**
   * Create new SinceDB instance from the position of the reader.
   *
   * @param instance of SinceDB to update
   * @param reader the reader that needs to be queried.
   * @return new instance of SinceDB containing the new position.
   */
  static SinceDB getUpdated(SinceDB oldSinceDb, DeadLetterQueueReader reader);
}

The lack of private static methods in interfaces in Java 8 makes this more annoying than it would be in Java 11 though...

[andsel]

That's nicer, applied with commit 5befc7e

[robbavey]

Is there a reason you're setting the current segment to user.home? This doesn't seem like it can ever be the case?
Or is this to ensure that retrieveNewPosition can work as a method to construct a SinceDB object by using its local copy of sinceDbPath?

[andsel]

I used user.home as a no-op value, could it be a /tmp also. It's only used so that the object constructed and is able to create the new instance in retrieveNewPosition

[robbavey]

Is there a reason why you moved from the Optional usage to the Null object pattern? The Null Object pattern here introduces some complexity, including the use of a tricky inheritance, which requires constructing the superclass with "bogus" parameters, such as Paths.get(System.getProperty("user.home"), and requires this method that "looks" like a mutation?

[robbavey]

LGTM