Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

initial pass at splitting the project into modules #69

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

agaro1121
Copy link
Contributor

@agaro1121 agaro1121 commented Sep 30, 2018

Overview

This is an attempt to split the project into modules. All modules are under the modules/ directory
The modules are:

  1. core models
  2. The consumer Actor + stream impl
  3. The producer Actor + stream impl
  4. Integration Tests - made integration tests it's own project so Travis switches to it to run the integration tests specifically
  • metaspace increased to 512mb
  • updated build.sbt to account for all the modules
  • updates scala (2.11.12)
  • includes updated dependencies from previous release

TODO

- separate consumers and producers in unit tests i.e producer test have a simple consumer and vice versa
- fix issue where KinesisSuite does not load core's reference.conf at the right time

Status

- it module still needs work to compile

  • compiles and all tests passing (unit and integration)
  • Travis Build passing

need to separate consumers and producers from each others tests
Copy link
Contributor

@markglh markglh left a comment

Choose a reason for hiding this comment

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

Good start!
I'm more leaning towards having it as a top level module rather than within each one. That'll make it much easier to work with and probably fix the struggles!?

@@ -14,7 +14,7 @@
* limitations under the License.
*/

package com.weightwatchers.reactive.kinesis.stream
Copy link
Contributor

Choose a reason for hiding this comment

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

why com.ww.com?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops! Fixed!

project
.in(file("."))
.in(file("core"))
.settings(name := "core")
Copy link
Contributor

Choose a reason for hiding this comment

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

can we nest them all in modules/xxx

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be a little cleaner to have all the code modules nested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

build.sbt Outdated
"ch.qos.logback" % "logback-classic" % "1.2.3" % Compile
val producer =
project
.in(file("producer"))
Copy link
Contributor

Choose a reason for hiding this comment

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

as above :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/**
* Main entry point for creating a Kinesis source and sink.
*/
object Kinesis extends LazyLogging {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth renaming to make it clear which is the consumer and which is the producer (vs just the package?) - I'm torn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it's worth renaming. Don't ask me what that name should be though. Naming things is for the high wizards LOL

This is a tough one though.
It's a combination of consumer/producer + stream + source/sink
Looks like the old factory pattern to me. Should we go around those lines?

Copy link
Contributor Author

@agaro1121 agaro1121 Feb 9, 2019

Choose a reason for hiding this comment

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

I named these:
ConsumerStreamFactory and ProducerStreamFactory
I'm open to suggestions

makes it a top-level module
@agaro1121
Copy link
Contributor Author

agaro1121 commented Feb 9, 2019

@markglh

  • I addressed your comments regarding the modules directory
  • it is its own module now. That solved a lot of compile issues. Thank you.

TODO

  • The consumer/producer stream factories still need renaming
    - it module needs some work still

build.sbt Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Feb 9, 2019

Coverage Status

Coverage increased (+0.3%) to 90.435% when pulling 8515a91 on feature/split-modules into b8240cf on master.

* These libs are included with the Amazon dependencies
* Our core module does not include amazon dependencies so we need to bring them in manually
* These versions match the ones in the Amazon dependencies
* */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markglh Not sure how future-proof this is as we continue to update Amazon libs
Is there a better way to do this?

project
.in(file("."))
.settings(name := "reactive-kinesis")
.aggregate(core, consumer, producer)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't include it here
i'm assuming we don't need to

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

Successfully merging this pull request may close these issues.

3 participants