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.

Sign up for GitHub

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

Jump to bottom

Adds a read-side processor to shopping-cart (Java) #60

Merged
merged 7 commits into from
Sep 6, 2019
Merged

Adds a read-side processor to shopping-cart (Java) #60

merged 7 commits into from
Sep 6, 2019

Conversation

octonato
Copy link
Member

@octonato octonato commented May 25, 2019 •
edited
Loading

TODOS:

  • tests for read-side
  • integration tests on Central Park (just hit the url once to confirm it's working)
  • json format for date

Currently displaying...

{
    "checkoutDate": 1558773849.37,
    "creationDate": 1558773801.879,
    "id": "1234"
}

@ignasi35
Copy link
Contributor

json format for date

I don't think this should be demoed in this sample app. We should keep the scope of shopping-cart as small as possible.

@octonato octonato changed the title [WIP] Adds a read-side processor to shopping-cart (Java) Adds a read-side processor to shopping-cart (Java) May 27, 2019
ignasi35
ignasi35 reviewed May 28, 2019
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
@octonato octonato force-pushed the read-side-processor-java branch 3 times, most recently from 7b38cce to e0da979 Compare May 28, 2019 13:38
@octonato octonato closed this May 28, 2019
@octonato octonato reopened this May 28, 2019
@TimMoore
Copy link

TimMoore commented Jun 5, 2019

Could this be split up so that this pull request is only a port of #57 + #65, with the build changes extracted to a separate pull request (maybe deferred)?

@octonato
Copy link
Member Author

octonato commented Jun 5, 2019

I will do it

@octonato octonato mentioned this pull request Jun 6, 2019
Merged
@octonato
Copy link
Member Author

octonato commented Jun 6, 2019

@TimMoore, I created #66 with only the build scripts. I don't want to defer it because the changes make it easier to advance this PR.

I will adapt this PR accordingly.

@ignasi35 ignasi35 mentioned this pull request Jul 26, 2019
Closed
20 tasks
@octonato octonato mentioned this pull request Jul 31, 2019
Closed
@octonato
Copy link
Member Author

The tests are still flaky and won't improve unless we fix lagom/lagom#2140.

@dwijnand dwijnand mentioned this pull request Aug 12, 2019
Closed
12 tasks
@marcospereira marcospereira mentioned this pull request Aug 28, 2019
Closed
11 tasks
@octonato octonato mentioned this pull request Aug 30, 2019
Merged
@octonato
Copy link
Member Author

octonato commented Sep 4, 2019 •
edited by dwijnand
Loading

lagom/lagom#2140 is fixed and backported to 1.5.x

This branch is now using Lagom 1.5.3 which has the fix. It should now pass all the tests.

marcospereira
marcospereira approved these changes Sep 4, 2019
View reviewed changes
Copy link
Contributor

@marcospereira marcospereira left a comment

Choose a reason for hiding this comment

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

@renatocaval made two small commits.

LGTM.

@marcospereira
Copy link
Contributor

@renatocaval build breaks are legit:

The database tables aren't being created when running the tests.

octonato
octonato commented Sep 6, 2019
View reviewed changes
...t-java/shopping-cart/src/test/java/com/example/shoppingcart/impl/ShoppingCartReportTest.java Outdated
@@ -56,8 +56,6 @@ public static void afterAll() {
public void createAReportOnFirstEvent() throws InterruptedException, ExecutionException, TimeoutException {

String cartId = UUID.randomUUID().toString();
assertNull(Await.result(reportRepository.findById(cartId)));
Copy link
Member Author

Choose a reason for hiding this comment

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

This kind of assertions was causing the build to eventually fail. When we reach that line, it's possible that the read-side processor didn't get a chance to create the user tables and then it will fail on executing this selection.

I'm removing it because it's not strictly necessary for the tests. However, we need to rethink the test driver. Users can easily run into that situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened an issue to address this.
lagom/lagom#2248

@octonato octonato mentioned this pull request Sep 6, 2019
Open
@mergify mergify bot merged commit b71b167 into 1.5.x Sep 6, 2019
@mergify mergify bot deleted the read-side-processor-java branch September 6, 2019 22:16
@octonato octonato mentioned this pull request Sep 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ignasi35 ignasi35 ignasi35 left review comments

@marcospereira marcospereira marcospereira approved these changes

Assignees
No one assigned
Labels
status:needs-forwardport
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@octonato @ignasi35 @TimMoore @marcospereira