Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pipeline common tests #36

Merged
merged 34 commits into from
May 27, 2020
Merged

Pipeline common tests #36

merged 34 commits into from
May 27, 2020

Conversation

daveter9
Copy link
Contributor

Did a refactor which allows for dependency injection.

The PipelinePush and PipelinePull interfaces use PeerConnectionInformation to represent the config data required to set up a connection. The goal of these interfaces is to abstract away connection (ZMQ) specific behavior, since other connection implementations may require a different implementation of PeerConnectionInformation the current proposed refactor is not satisfying. How can this config be completely decoupled from the concrete connection Implementation?

@daveter9 daveter9 added help wanted Extra attention is needed backend Related to development of the backend aggregate server labels May 20, 2020
@daveter9 daveter9 added this to the First draft SIG milestone May 20, 2020
@daveter9 daveter9 self-assigned this May 20, 2020
@daveter9 daveter9 marked this pull request as draft May 20, 2020 16:22
@nickyu42
Copy link
Contributor

From what i understand, the problem is that concrete implementations of PipelinePush and PipelinePull need their own config type to set up a connection, such as PipelinePullZMQ needing PeerConnectionInformation. Meaning that you want to completely remove the concrete config type from the PipelinePush and PipelinePull interfaces.

I think you could make it generic, if you really want to decouple it.

Example for PipelinePull:

PipelinePull.kt

// Add some constraints?
interface GenericConfig

interface PipelinePull<T : GenericConfig> {
    ...
    // Make config generic
    fun setupConnection(config: T)
    ...
}

AbstractPipelinePlugin.kt

data class PeerConnectionInformation(
    val host: String,
    val isBind: Boolean
) : GenericConfig

PipelinePull.kt

class PipelinePullZMQ : PipelinePull<PeerConnectionInformation> {
    
    // The type signature of the concrete class has the 
    // concrete implementation of the necessary config
    override fun setupConnection(config: PeerConnectionInformation) {
        when (config.isBind) {
            true -> socket.bind(config.host)
            false -> socket.connect(config.host)
        }
    }
    ...
}

@daveter9 daveter9 removed the help wanted Extra attention is needed label May 25, 2020
@daveter9 daveter9 marked this pull request as ready for review May 25, 2020 10:22
@daveter9
Copy link
Contributor Author

daveter9 commented May 25, 2020

Achieved 50% Branch and 70% line coverage,
inline functions cannot be marked as covered by Jacoco (jacoco/jacoco#654)

@daveter9 daveter9 changed the title [WIP] Pipeline common tests Pipeline common tests May 25, 2020
@nickyu42
Copy link
Contributor

I think ../pipeline/PipelinbePluginConfigurationTest.kt has a typo

@daveter9
Copy link
Contributor Author

I think ../pipeline/PipelinbePluginConfigurationTest.kt has a typo

I will fix this

@nickyu42
Copy link
Contributor

nickyu42 commented May 26, 2020

It seems you used colons (:Class:) to link classes and functions in your documentation, which doesn't automatically generate links i think.
From what I know, neither KDoc ([Class]) or JavaDoc ({@link Class}) use this convention.

Should we decide on a convention to use for documentation? I don't really mind really

@daveter9
Copy link
Contributor Author

daveter9 commented May 27, 2020

  • Catch errors
  • conflicting files

@@ -8,6 +8,9 @@ include("plugin")
include("pluginmanager")
include("pipeline")
include("pipeline:common")
include("pipeline:plugins:extractor")
include("pipeline:plugins:pathextractor")
include("pipeline:plugins:renamer")
include("pipeline:plugins:renamer")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamer is included twice.

@daveter9 daveter9 merged commit 9b23a60 into master May 27, 2020
@daveter9 daveter9 added this to 🎉 Done in monitoring-aware-ides via automation May 27, 2020
@nickyu42 nickyu42 deleted the pipeline-common-tests branch May 29, 2020 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to development of the backend aggregate server
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants