DSLSpout and bullet-core-0.6.5 #69

0aix · 2019-01-11T22:49:38Z

No description provided.

coveralls · 2019-01-11T22:54:28Z

Coverage increased (+0.2%) to 99.58% when pulling b976497 on 0aix:master into c34cd85 on bullet-db:master.

akshaisarma

Need to add a setting to enable its usage and wire it in the Topology main class. Can we bump the version to 0.9.0?

akshaisarma · 2019-01-11T22:58:35Z

src/main/java/com/yahoo/bullet/storm/DSLSpout.java

@@ -0,0 +1,136 @@
+/*
+ *  Copyright 2019, Verizon Media.


Use the same one as the rest of the project or at least Oath. This is not valid yet.

I honestly didn't remember putting those in..

akshaisarma · 2019-01-11T22:59:50Z

src/main/java/com/yahoo/bullet/storm/DSLSpout.java

+     *
+     * @param args A list of arguments where the first argument is expected to be the path to a configuration file.
+     */
+    public DSLSpout(List<String> args) {


This constructor isn't needed. Even for testing.

akshaisarma · 2019-01-11T23:00:56Z

src/main/java/com/yahoo/bullet/storm/DSLSpout.java

+    @Override
+    public void nextTuple() {
+        List<Object> objects = Collections.emptyList();
+        try {


Push to a different method.

akshaisarma · 2019-01-11T23:02:22Z

src/main/java/com/yahoo/bullet/storm/DSLSpout.java

+        } catch (BulletDSLException e) {
+            log.error("Could not read from BulletConnector.", e);
+        }
+        long time = System.currentTimeMillis();


This time is a in-between time from reading to converting. Can we make it pre-emission after all conversion? Or pre-reading? Or get a new time per emission?

akshaisarma · 2019-01-11T23:03:03Z

src/main/java/com/yahoo/bullet/storm/DSLSpout.java

+        }
+        long time = System.currentTimeMillis();
+        for (Object object : objects) {
+            try {


Push to a new method and can use streams with filters for non-null. Also the try catch just needs to be around the convert call.

akshaisarma · 2019-01-11T23:03:41Z

src/main/java/com/yahoo/bullet/storm/DSLSpout.java

+
+    @Override
+    public void ack(Object id) {
+


Remove empty line here and below

akshaisarma · 2019-01-11T23:04:33Z

src/test/java/com/yahoo/bullet/storm/testing/MockConnector.java

+     * @param config Not used.
+     */
+    public MockConnector(BulletConfig config) {
+


akshaisarma · 2019-01-11T23:09:46Z

pom.xml

@@ -96,6 +97,11 @@
            <artifactId>bullet-core</artifactId>
            <version>${bullet.core.version}</version>
        </dependency>
+        <dependency>
+            <groupId>com.yahoo.bullet</groupId>
+            <artifactId>bullet-dsl</artifactId>


This will pull in kafka, pulsar and whatever else clients in the future. Can we use the DSL by having the jar at runtime and make it provided here? We need to try that out in storm.

Ok. I've tried this out. A couple problems but works for the most part.

I get a JNI error from the bullet-storm fat jar. I fixed this by making bullet-dsl "provided" and giving the jar to storm separately.

There's a pulsar dependency in BulletDSLConfig. Oops.

BulletConnector isn't serializable. Oops.

Let's fix bullet-dsl before merging this then

0aix · 2019-01-16T23:44:03Z

What I'm thinking to enable DSLSpout in the main class:

Set the original "bullet-spout" arg to optional
Add an optional "bullet-spout-use-dsl" boolean arg (or some other name)
(Later on) add other args for the dsl spout/bolt options we talked about
DSLSpout will use the same parallelism/cpu/memory args as "bullet-spout"
Rewrite the help message to make more sense

Edit: why do we use command line args anyways?

akshaisarma · 2019-01-17T00:15:37Z

Maybe we shouldn't hijack bullet-spout to do the dsl plugin. Let's just make a set of different settings that are only used if dsl is enabled. We should also have a setting to separate using the BulletConnector in the DSLSpout and the BulletConvertor in a new DSLBolt. If this setting is false, we do both in the DSLSpout.

akshaisarma · 2019-01-30T02:39:26Z

src/main/java/com/yahoo/bullet/storm/BulletStormConfig.java

+                 .defaultTo(DEFAULT_DSL_SPOUT_ENABLE)
+                 .checkIf(Validator::isBoolean);
+        VALIDATOR.define(DSL_SPOUT_PARALLELISM)
+                .checkIf(Validator::isPositiveInt)


Indentation on these and below

akshaisarma

There's a reference to as a Spout (in which case, just use the {@link Topology#main(String[])} method with the class name of your Spout. in the javadocs in StormUtils.java that is no longer applicable.

akshaisarma · 2019-02-06T23:21:29Z

src/main/java/com/yahoo/bullet/storm/DSLSpout.java

+            collector.emit(new Values(objects, System.currentTimeMillis()), DUMMY_ID);
+            return;
+        }
+        objects.forEach(this::convertAndEmit);


Can do collector.emit(new Values(dslBoltEnable ? objects : convert(objects), System.currentTimeMillis()), DUMMY_ID); and change convertAndEmit

My bad. List emit vs object emit

akshaisarma · 2019-02-06T23:22:25Z

src/main/java/com/yahoo/bullet/storm/StormUtils.java

    }

    /**
-     * This submits a topology after loading the given spout with the given configuration as the source of
-     * {@link com.yahoo.bullet.record.BulletRecord}.
+     * This submits a topology after loading the configured Spout (and optionally, bolt), which is either the {@link DSLSpout}


Capitalize bolt?

DSLSpout

331c25b

akshaisarma requested changes Jan 11, 2019

View reviewed changes

pr comments

9d0aaaf

0aix added 5 commits January 18, 2019 12:56

tests

124a5ac

javadocs

feecb3d

trying to get coverage

106f54d

removed bullet bolt

4b9c125

connector not initialized/closed during activate/deactivate in DSLSpout

a1c9bfb

akshaisarma requested changes Jan 30, 2019

View reviewed changes

0aix added 8 commits January 31, 2019 14:24

indentation; renaming; fat jar manifest fix

ba36887

added BulletDeserializer support in DSL

ca52954

bullet-dsl version

bca08f2

update help message

1df0a62

rl comments

48f090c

bullet-core version

10921b0

newline

72f55a8

bullet-dsl-0.1.1

73174bd

akshaisarma requested changes Feb 6, 2019

View reviewed changes

javadocs

b976497

akshaisarma approved these changes Feb 6, 2019

View reviewed changes

0aix merged commit 4287094 into bullet-db:master Feb 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DSLSpout and bullet-core-0.6.5 #69

DSLSpout and bullet-core-0.6.5 #69

0aix commented Jan 11, 2019

coveralls commented Jan 11, 2019 •

edited

Loading

akshaisarma left a comment

akshaisarma Jan 11, 2019

0aix Jan 16, 2019

akshaisarma Jan 11, 2019

akshaisarma Jan 11, 2019

akshaisarma Jan 11, 2019

akshaisarma Jan 11, 2019

akshaisarma Jan 11, 2019

akshaisarma Jan 11, 2019

akshaisarma Jan 11, 2019

0aix Jan 16, 2019

akshaisarma Jan 18, 2019

0aix commented Jan 16, 2019 •

edited

Loading

akshaisarma commented Jan 17, 2019

akshaisarma Jan 30, 2019

akshaisarma left a comment

akshaisarma Feb 6, 2019

akshaisarma Feb 6, 2019

akshaisarma Feb 6, 2019

DSLSpout and bullet-core-0.6.5 #69

DSLSpout and bullet-core-0.6.5 #69

Conversation

0aix commented Jan 11, 2019

coveralls commented Jan 11, 2019 • edited Loading

akshaisarma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0aix commented Jan 16, 2019 • edited Loading

akshaisarma commented Jan 17, 2019

Choose a reason for hiding this comment

akshaisarma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 11, 2019 •

edited

Loading

0aix commented Jan 16, 2019 •

edited

Loading