JQL refactoring draft #23
Conversation
drspartez
requested review from
wyrzyk,
dagguh,
mcmurphy2000,
sebapawlak and
lipinskipawel
| import com.atlassian.performance.tools.jiraactions.api.SeededRandom | ||
| import com.atlassian.performance.tools.jiraactions.api.page.IssuePage | ||
|
|
||
| interface JqlContext { |
There was a problem hiding this comment.
Looks like and public SPI. Historically we had many issues with SPI written in Kotlin and how it interoperated with Java and Java interfaces and their default methods. I would suggest writhing this part in Java which should not be costly but may pay off in case we want to extend the SPI in the future without breaking the compatibility.
| import org.apache.logging.log4j.Logger | ||
| import java.util.function.Predicate | ||
|
|
||
| class AdaptiveJqlMemory2( |
There was a problem hiding this comment.
Why this class is public? We have already public interface JqlMemory2.
Should we create some sort of factory that returns the instance of JqlMemory2 in this case AdaptiveJqlMemory2?
What should be a general idea here?
There was a problem hiding this comment.
in general classes with "2" suffix are replacements, for existing which will be deprecated and removed with major change, so in some time number of interfaces will be the same as before refactoring
There was a problem hiding this comment.
@lipinskipawel for more invasive refactoring You can look here
https://github.com/drspartez/jira-actions/tree/issue/memories-refactoring
A lot of implementation is hidden there. And memory interfaces are reduced to 2 pieces
There was a problem hiding this comment.
My question was more about the Factory class, I do understand the "2" suffix.
Anyway, good to see Factory in the commit you sent.
There was a problem hiding this comment.
OK. So concern of this change is to replace "String" jql representation with something else which is more relevant (so all JQL interfaces ore for this reason). Other stuff remains almost untouched (so we still offer Adaptive memories implementations as usual).
In second approach (https://github.com/drspartez/jira-actions/tree/issue/memories-refactoring) i changed the fashion we provide adaptive memories (hidden implementation and factories), also reduced number of actual memory interfaces to 2.
There was a problem hiding this comment.
Why this class is public?
The old AdaptiveJqlMemory was also API, so it's perfectly fine for the replacement to be public as well.
It was published to make custom scenarios easier to write.
There was a problem hiding this comment.
Should we create some sort of factory that returns the instance of JqlMemory2 in this case AdaptiveJqlMemory2?
Factories cost complexity and must have additional value to be justified. AFAIK across all JPT libs we expose only one factory: com.atlassian.performance.tools.report.api.result.RawCohortResult.Factory. We did that to offer different kinds of RawCohortResult without allowing custom subtypes.
Why did we forbid custom subtypes? Because we needed to add methods to the (old) CohortResult, but we didn't have a pattern for that. So we created a replacement (just like JqlMemory2) and we wanted to protect ourselves from this problem in the future. We decided nobody should need to implement this interface. In short, we wanted RawCohortResult to be API, but not SPI.
The question is - do we want to allow custom implementations of JqlMemory2? If so, let's go for a factory, else public constructors/builders.
| fun issuePage(): IssuePage? | ||
| } | ||
|
|
||
| interface JqlSupplier { |
There was a problem hiding this comment.
Is it possible to reduce the number of public interfaces? It seems that our API grows quickly.
There was a problem hiding this comment.
supplier is replacement for PrescriptionType which is public and could be removed only witch major change
There was a problem hiding this comment.
It's fine to have many small interfaces in the API. It's much better than having a few huge ones.
| fun issuePage(): IssuePage? | ||
| } | ||
|
|
||
| interface JqlSupplier { |
There was a problem hiding this comment.
It's fine to have many small interfaces in the API. It's much better than having a few huge ones.
| BuiltInJQL.GENERIC_WIDE.get(JqlContextImpl(random))?.let { preparedQueries.add(it) } | ||
| } | ||
|
|
||
| private val availableJqlSuppliers = mutableSetOf( |
There was a problem hiding this comment.
Memory impls are implicitly allowed to be not thread-safe, because each VU has its own memories.
It's a nice opportunity to make that assumption explicit and mark AdaptiveJqlMemory2 as @NotThreadSafe.
| import org.apache.logging.log4j.Logger | ||
| import java.util.function.Predicate | ||
|
|
||
| class AdaptiveJqlMemory2( |
There was a problem hiding this comment.
Why this class is public?
The old AdaptiveJqlMemory was also API, so it's perfectly fine for the replacement to be public as well.
It was published to make custom scenarios easier to write.
| import org.apache.logging.log4j.Logger | ||
| import java.util.function.Predicate | ||
|
|
||
| class AdaptiveJqlMemory2( |
There was a problem hiding this comment.
Should we create some sort of factory that returns the instance of JqlMemory2 in this case AdaptiveJqlMemory2?
Factories cost complexity and must have additional value to be justified. AFAIK across all JPT libs we expose only one factory: com.atlassian.performance.tools.report.api.result.RawCohortResult.Factory. We did that to offer different kinds of RawCohortResult without allowing custom subtypes.
Why did we forbid custom subtypes? Because we needed to add methods to the (old) CohortResult, but we didn't have a pattern for that. So we created a replacement (just like JqlMemory2) and we wanted to protect ourselves from this problem in the future. We decided nobody should need to implement this interface. In short, we wanted RawCohortResult to be API, but not SPI.
The question is - do we want to allow custom implementations of JqlMemory2? If so, let's go for a factory, else public constructors/builders.
| .filter { it != null } | ||
| .firstOrNull() | ||
|
|
||
| jql?.let { |
There was a problem hiding this comment.
Micro-code-style 😉
jql can be inlined and therefore chained
and then the it can be renamed to jql
| override fun supplier(): JqlSupplier = this.supplier | ||
| } | ||
|
|
||
| internal class JqlContextImpl(private val rnd: SeededRandom, private val page: IssuePage?) : JqlContext { |
There was a problem hiding this comment.
rnd - Research and Development? 
| } else { | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
There's something wonky about the indents here
| import com.atlassian.performance.tools.jiraactions.api.jql.JqlSupplier | ||
| import com.atlassian.performance.tools.jiraactions.api.page.IssuePage | ||
|
|
||
| internal class JqlImpl(private val query: String, |
There was a problem hiding this comment.
"JQL implementation". What makes this one different from the other implementations?
If none other exist, how could they differ?
It seems that the interface allows dynamic query and supplier logic, but this impl simply reuses the provided fields. Suggestions: StaticJql, SuppliedJql.
| import com.atlassian.performance.tools.jiraactions.api.page.IssuePage | ||
| import java.util.function.Predicate | ||
|
|
||
| interface JqlMemory2 : Memory<Jql> { |
There was a problem hiding this comment.
If this a replacement for JqlMemory, shouldn't that one be deprecated?
If it's too early to deprecate, then maybe it's too early to put in replacements? New API works best if it was tried in some way, e.g.:
- unit/integration tests within the module
- equivalent API used outside of the module
In the meantime i baked JQL related refactoring proposal.
I've divided JQL domain into 3 public interfaces:
There are also public JqlMemory2, AdaptiveJqlMemory2 and (names just to distinguish from previous)
Other related stuff is implementation