Skip to content
This repository has been archived by the owner on May 9, 2019. It is now read-only.

Move CassandraReadSideUtils and CompletionStageUtils to core #74

Open
yg-apaza opened this issue Mar 27, 2017 · 11 comments
Open

Move CassandraReadSideUtils and CompletionStageUtils to core #74

yg-apaza opened this issue Mar 27, 2017 · 11 comments

Comments

@yg-apaza
Copy link
Contributor

yg-apaza commented Mar 27, 2017

./item-impl/src/main/java/com/example/auction/item/impl/CassandraReadSideUtils.java:11:
// TODO: move to core?
./tools/src/main/java/com/example/core/CompletionStageUtils.java:14:
// TODO: move to core?

Move CassandraReadSideUtils (item-impl) and CompetionStageUtils (tools) into the core Lagom framework

@lakhina
Copy link
Contributor

lakhina commented Mar 29, 2017

@yg-apaza Are you working on it?

@yg-apaza
Copy link
Contributor Author

yg-apaza commented Mar 29, 2017

@lakhina No, you can work on this :-)

@TimMoore
Copy link
Contributor

@ignasi35 can correct me if I'm wrong, but I think the "move to core" comments were about moving them into the core Lagom framework to be used by all projects, not just about moving them to the com.example.core package in this project. That's why the comment also appears in CompletionStageUtils.

I think there is a discussion we still need to have amongst the Lagom development team about how we should move useful utilities into the framework.

@ignasi35
Copy link
Contributor

At @TimMoore pointed out, moving features from online-auction (or other sample apps) into lagom core is not a very clear process and we don't have a clear decision process to determine what's a good addition and what isn't.

In the past we've only moved one feature and the steps were:

  • extract code from all the location in online-auction where it's applied/copy-pasted and move it to tools/. This step may require some API tuning and discussing. tools/ is a launch platform into Lagom. Preferably, this step includes adding tests to the extracted feature.
  • in Lagom, copy/paste code from online-auction/tools. If applicable, include integration-tests, scripted tests, other... . Include Javadocs and user documentation.

This process doesn't cover when/how to implement the feature in the other languages. That is, if refactoring code online-auction-java, it could be necessary that we do the same in online-auction-scala because when moving the code into Lagom we'll have to keep feature parity.
Sometimes a feature only applies to java (or scala) dues to language-specifics features/limitations and sometimes the online-auction applications are not up to date with each other in terms of development and providing a feature parity requires updating other areas first (Yak-Shave All TheThings!).

@ignasi35
Copy link
Contributor

Also:

./item-impl/src/main/java/com/example/auction/item/impl/CassandraReadSideUtils.java:11:
./tools/src/main/java/com/example/core/CompletionStageUtils.java:14:

these two files were originally one file used by several classes. One such class was promoted into Lagom via tools/ and that forced me to move CompletionStageUtils from its original location into tools/. Then I realised a method required a dependency to Cassandra and I extracted that out and put it back into a new file CassandraReadSideUtils.

The process of launching code via tools/ helped me detected there were two different things into a single file and also surfaced the dependencies I would need when moving that bit into Lagom.

@TimMoore TimMoore changed the title Move to core Move CassandraReadSideUtils and CompletionStageUtils to core May 15, 2017
@TimMoore
Copy link
Contributor

See also lagom/lagom#732

@yg-apaza
Copy link
Contributor Author

Can we start by moving CassandraReadSideUtils and also com.example.auction.item.impl.testkit.* to com.example.core? Then, we can just wait until a decision is made in Lagom.
I think we can use tools for both reasons: as a common project and as a launch platform to core.
There have been some times when I was in the need of using these utilities in Transaction service (This integration test is repeating Await.result)

@TimMoore
Copy link
Contributor

@yg-apaza makes sense to me

Maybe we should keep the testkit subpackage for the classes there. (So, com.example.core.testkit.

Also, would it be better to call it com.example.tools since it's in a project called tools? @ignasi35 any opinion on this?

@ignasi35
Copy link
Contributor

Even if the project is named tools I think packages should be as specific as possible (and preferably avoid the tools naming altogether). I think an option is to use a packagename as close as possible to what we're aiming when we move the code to core.

I think we can use tools for both reasons: as a common project and as a launch platform to core.

I chose the name of the project very poorly and now we're getting these confusions. I think there's room in online-auction for a project with common code, but when I created tools I only meant it as a launch platform into core. For example, security is also a project for common code but it's purpose is very specific. That is a good name.

Final note: this issue originally mentioned ReadSideTestDriver and CompletionStageUtils. Both have their own issues in lagom/lagom already: lagom/lagom#421 for ReadSideTestDriver and lagom/lagom#732 for CompletionStageUtils.

@TimMoore
Copy link
Contributor

@ignasi35 what would you suggest as an immediate solution to the problem of sharing code between multiple Online Auction services?

Maybe it makes sense to introduce a new testkit subproject... after al, we probably don't want to mix that code into the runtime classpath of the services.

@ignasi35
Copy link
Contributor

IMHO, ReadSideTestDriver should be in tools. We know it'll eventually jump into core.
But I agree with you that scoping had not been necessary in the past and it is now. So we could put ReadSideTestDriver in tools/src/main/test and have other projects depend on tools % "test-> test" (not sure that's the correct syntax).

Actually, I think TopicStub was in tools before making it's way into core and it was in tools/Src/main. What I'm trying to say is that in the past we already had testkit code visible from compile scope for few days/weeks. Hmmm, maybe we should get this scoping thing addressed already, it's becoming a habit to ignore this scoping issue.

Long-story short: I would move ReadSideTestDriver into tools/src/test, fix the dependencies and prioritize it's promotion into core so that ReadSideTestDriver stays in tools the shortest period we can.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

5 participants