Spark dependencies as child of 'dependencies'

[jpkrohling]

Follow-up from #140: the spark dependencies spec type is currently a child of the storage spec type. While it might be OK if we only ever have Spark to process the dependency graph, it would probably be better to have a 'dependency spec' type with a 'spark dependency spec' child. This way, we could easily add other dependency processors in the future.

cc @pavolloffay, @objectiser

[pavolloffay]

+1 I don't like to expose spark-dependencies properties directly in as storage.dependencies.*.

However, I don't see benefits scoping it in a separate object under dependencies like storage.dependencies.spark.* for the following reasons:

• it's harder to write configuration files when it's scoped under 2 nested objects
• we have only a single implementation of dependencies graph, even not sure if there will be other in future considering we might move to latency diagrams
• hard to say that deps implementation would share any common properties

The most I like is storage.sparkDependencies it's directly related to project what is being used. it's easy to configure and deprecate in the future. Overall I think about it as a separate unit which operates independently on the storage.

[objectiser]

@jpkrohling @pavolloffay Sorry didn't pick this up on the original review, but the spark PR has elaborated all of the (additional) storage properties in the CRD model - which is different to how the storage options work.

One suggestion - we could simply have:

storage:
  dependencies:
    enabled: true
    options:
      es:
        client-mode-only: true
      spark:
        master: xyz
      java-opts: .....

(or dependencies at the top level)

Another possibility would be to just have the options included under the storage.options? It would mean they would also be passed to query/collector as flags - would this cause Viper to throw error due to unknown flag?

[pavolloffay]

Downside of using poperies is documentation and perhaps type validation. With properties we will not be able to auto-generate docs.

Another possibility would be to just have the options included under the storage.options

Seems confusing to me.

[objectiser]

Downside of using poperies is documentation and perhaps type validation. With properties we will not be able to auto-generate docs.

Agree - but that is the approach currently being taken in the operator, so from a consistency approach it would be better.

[pavolloffay]

Agree - but that is the approach currently being taken in the operator, so from a consistency approach it would be better.

I do not see that consistency. I think it's used only for jaeger services (query, collector) where the props are passed as flags because these services support flags which makes easy to use map and pass it to flags.

For instance JaegerCassandraCreateSchemaSpec does not use properties and expose its configuration as object fields.

// JaegerCassandraCreateSchemaSpec holds the options related to the create-schema batch job
type JaegerCassandraCreateSchemaSpec struct {
	Enabled    *bool  `json:"enabled"`
	Image      string `json:"image"`
	Datacenter string `json:"datacenter"`
	Mode       string `json:"mode"`
}

Edit: Using properties map makes sense if binary supports flags(cmd parameters).This gives the flexibility to bump component version (assumably with new flags) without changing operator.

Mapping env vars to prop map loses docs and type safety.

[objectiser]

Yes, forgot about the cassandra schema job.

[pavolloffay]

Closing, we don't want to introduce a breaking change to the CR. There are no requests to create new dependencies implementation.