[WIP] Implement refactoring changes

[aaron-congo]

Note: will be merged into the refactoring feature branch, not the main branch

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

Qodana Community for JVM

1 new problem were found

Inspection name	Severity	Problems
Mismatched query and update of collection	🔶 Warning	1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

1. Register at Qodana Cloud and configure the action
2. Use GitHub Code Scanning with Qodana
3. Host Qodana report at GitHub Pages
4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/[email protected]
        with:
          upload-result: true

Contact Qodana team

Contact us at [email protected]

• Or via our issue tracker: https://jb.gg/qodana-issue
• Or share your feedback: https://jb.gg/qodana-discussions

[aaron-congo]

Once we remove the suggested ID functionality we can assess whether we want to remove the getEntries method as well

[aaron-congo]

I removed this because it is not being called anywhere and because I am using dependency injection to pass in a StorageService instance. Since the StorageService is an instance variable it cannot be accessed via a static context, so we cannot use it in this static method.

[aaron-congo]

This class is similar to SlidingExpirationCacheWithCleanupThread except it takes in an expiration timeout in the constructor instead of with each get/set method. It can hold either items that do not have renewable expiration or items that do, so it can be used to replace both SlidingExpirationCacheWithCleanupThread or CacheMap. Alternatively we could implement a renewable expiration cache and a non-renewable expiration cache separately, at the expense of some code duplication.

[aaron-congo]

The get() method requires that you pass a Class specifying what the expected value is. This is needed inside the method to check if it matches what is in the cache and cast it for the caller. The limitation with this is that the caller cannot specify a generic class. For example, there is no way for the caller to specify specifically that they want a List. They can specify that they want a List.class but that still does not let us properly check if the List is a List. The solutions to this are:

1. Use a non-generic class to wrap the generic object, which is what I've done here
2. Remove the expected class as a parameter to get(), and return an Object without casting or checking an expected type. In this case the caller would have to check if the returned Object was the correct type and cast it, which is annoying
3. Google Guava has a TypeToken dependency which may provide a way to specify and preserve the generic type info, but this requires adding it as a dependency which may not be acceptable

[aaron-congo]

I don't love how we partially initialize the ServiceContainer here and then complete the initialization with the setters below. I wonder if we should consider a proper service locator to avoid this.

[sergiyvamz]

There's nothing wrong about partially initialize the object. By design this object is supposed to hold service references. Some of them could be missed, i.e. not yet initialized. I think we 're fine. We could improve it later.

[aaron-congo]

I created the MonitorItem inner class so that monitors can be stored together with the suppliers used to create them. This allows us to re-create the monitor if it encounters an error or gets stuck. However, it makes things messy in this function. The caller passes in a shouldDisposeFunc for the monitor itself (eg ClusterTopologyMonitorImpl), but since we are actually storing MonitorItem's in the cache, we need to pass ShouldDisposeFunc<MonitorItem>. For now I resolved this by wrapping the passed in lambda, but this seems messy and I am getting a warning when I try to cast monitorItem.getMonitor() to T.

[sergiyvamz]

We have another instance of the service in Driver class. We should use a single one.

[aaron-congo]

The Driver and AwsWrapperDataSource are independent classes so there is not a way to pass the storage service from one to the other. The Driver class needs the storage service reference so that it can clear it when Driver.clearCaches is called. I think the only way we could make sure its a single instance is if we make it a singleton, do we want to do that? Keep in mind, even though AwsWrapperDataSource and Driver have different instances of StorageServiceImpl, the cache info is static so it will still be shared between both.

[sergiyvamz]

Does it make sense to add heartbeat functionality to this class? It'd be nice just to inherit it.

[aaron-congo]

That would be nice, I'm not sure how to add that though. Each monitor type will have its own custom run loop that it needs to define, and the last updated timestamp needs to be updated somewhere in that custom loop. Not sure how to extract the heartbeat piece to the abstract monitor while leaving the custom loop implementation to the underlying monitor class.