new adapter read and write api #248

demmer · 2016-01-22T21:24:50Z

This PR is a big rework of the implementation of read and write and the adapter API.

Instead instead of having adapters directly implement procs, add normal proc implementations of read and write along with a new set of abstract base classes for adapters that have a cleaner contract that is more isolated from the details of the rest of the runtime.

Also convert the test adapters and the file adapter to use the new API and add a new test adapter that simulates a timeseries database to test the logic for reading pseudo-live.

More details are in the individual commit messages.

Not yet ready for merge but initial review would be appreciated.

To do before merge:

Convert the file adapter
Convert the http adapter
Convert the stdio adapter
Convert the stochastic adapter
Rework emit so that ticks are generated by the source proc
Implement one of the external adapters using this contract

Fixes #168.

demmer · 2016-01-22T23:31:49Z

@juttle/developers

davidvgalbraith · 2016-01-24T02:44:25Z

lib/adapters/http/read.js

+
+        var format;
+        try {
+            format = contentType.parse(res).type.split('/')[1];


Know this was just a copy-paste but I'm confused by the logic...you take the content type of res and then use that to get a parser that you use on req? Seems like this should be contentType.parse(req).

Yeah I agree -- it turns out that either works, but it does seem confusing to parse the content type of the request and not the response.

I tightened things up so we don't use the req object at all any more.

demmer · 2016-01-25T22:33:09Z

@davidvgalbraith I addressed all your comments, rebased, and then also made some additional changes. Everything from 0c7b30e onward is new.

Seems like we're close enough that it would make sense to merge this sooner rather than later and then start the process of converting the adapters. Once this merges I'd probably want to cut a new juttle release so we have something to base the adapter rework off of.

davidvgalbraith · 2016-01-25T22:44:07Z

lib/runtime/procs/read.js

+            this.emit_eof();
+        } else {
+            this.reading.then(() => {
+                this.emit_eof();


This will emit 2 EOFs if the read hits an error due to line 237. I don't think that's a big deal, though.

Good point -- that doesn't seem like a good thing.
Even worse, I noticed that if there is a pending call into the scheduler that never gets cancelled or cleaned up.

davidvgalbraith · 2016-01-25T22:58:17Z

+1

davidvgalbraith · 2016-01-25T23:08:03Z

lib/runtime/juttle-utils.js

+
+    toNative(points);
+
+    return points;


This can just return toNative(points).

davidvgalbraith · 2016-01-27T08:42:39Z

lib/runtime/adapter-write.js

+    // Returns a promise once the adapter has completed writing all the data and
+    // the flowgraph can be shut down.
+    eof() {
+        throw new Error('write must be implemented');


Should be "eof must be implemented".

davidvgalbraith · 2016-01-27T23:16:03Z

lib/adapters/stdio/write.js


-    eof: function() {
+    eof() {


The contract says we're supposed to return a promise here.

The base class uses Promise.try so I didn't see any need here.

Well then we should change the contract. Are we synchronously done here after calling this.serializer.done() anyway?

Well that seems like a good thing to do.

So they are supported by any adapter.

Code coverage slipped a bit on functions due to the fact that the new read and write implementations have shim support for the old adapters that isn't being exercised as part of the tests. This can bump back up once we remove the legacy adapter support.

The previous model in which adapters would convey statically whether or not they should behave as a timed source or an untimed source proved too limiting in practice, since it didn't lend itself well to cases like influx or sql in which the behavior actually depends on the other options given to the read call. Instead, change things to give adapters more control over their desired behavior for time options by adding two new API hooks, defaultTimeRange() to return defaults for from and to, and periodicLiveRead() which indicates to the read proc whether to do the pseudo-live pattern for reading or leave it up to the adapter to manage its own paging through time.

The previous code was stomping any location info that the adapter put into the errors, so instead only add it if it's missing.

Add generic errors for procs to use when they encounter incompatible option combinations. Rework emit to use these instead of a bunch of one-off error strings.

The rework to use the base class ticking behavior conflicted with the concurrent change to not add timestamps to the -points argument, so reconcile these two changes.

demmer · 2016-01-29T19:57:59Z

@juttle/developers I took another pass at changing the API to better support adapters that wabt to determine their behavior relating to the time options at runtime instead of statically. I also rebased to reconcile all the latest changes to the various builtin adapters (thanks rodney) and addressed the various comments in the PR history.

I think this is ready to merge so we can get on with porting the actual adapters.

davidvgalbraith · 2016-01-30T00:21:05Z

lib/adapters/file/read.js

        })
-        .catch(function(err) {
-            self.trigger('error', self.runtime_error('RT-INTERNAL-ERROR', {
+        .catch((err) => {


This catch can go after the following then.

davidvgalbraith · 2016-01-30T00:34:19Z

+1

demmer · 2016-01-30T01:05:53Z

Dave said "ship it red!"

new adapter read and write api

rlgomes · 2016-01-30T03:15:03Z

I'll look at that file adapter test failure...

demmer added the in progress label Jan 22, 2016

demmer force-pushed the adapters-are-not-procs branch 2 times, most recently from 34a3346 to fbf8b66 Compare January 22, 2016 23:31

demmer force-pushed the adapters-are-not-procs branch 2 times, most recently from f43d8ad to 7841362 Compare January 23, 2016 00:18

davidvgalbraith reviewed Jan 24, 2016
View reviewed changes

demmer force-pushed the adapters-are-not-procs branch 2 times, most recently from 8542309 to 11e9769 Compare January 25, 2016 22:12

demmer mentioned this pull request Jan 25, 2016

source base class read logic #227

Closed

davidvgalbraith reviewed Jan 25, 2016
View reviewed changes

lib/runtime/juttle-utils.js

toNative(points);

return points;

Copy link

Contributor

davidvgalbraith Jan 25, 2016

Choose a reason for hiding this comment

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

This can just return toNative(points).

davidvgalbraith mentioned this pull request Jan 26, 2016

Add s3 adapter #196

Closed

davidvgalbraith reviewed Jan 27, 2016
View reviewed changes

VladVega mentioned this pull request Jan 27, 2016

add delete and rename for time #277

Closed

davidvgalbraith reviewed Jan 27, 2016
View reviewed changes

demmer force-pushed the adapters-are-not-procs branch 6 times, most recently from 7bcf95b to cf9a0bf Compare January 28, 2016 15:00

Mark Stemm and others added 13 commits January 29, 2016 11:55

Add -every and -lag as common options.

66edefd

So they are supported by any adapter.

CHANGELOG: add entries for recent changes to the adapter API

94920a7

file: update the file adapter to the new contract

a207d48

http: update the http adapter to the new contract

406335a

stdio: update the stdio adapter to the new contract

e91ee14

stochastic: update the stochastic adapter to the new contract

08bdcaa

emit: update emit to the new time option behavior

c1064a4

adapters: update comment in write to reflect reality

aacce8a

read: set the location on errors only if not supplied already

ec80e96

The previous code was stomping any location info that the adapter put into the errors, so instead only add it if it's missing.

errors: clean up and consolidate a bunch of invalid option errors

e3b1643

Add generic errors for procs to use when they encounter incompatible option combinations. Rework emit to use these instead of a bunch of one-off error strings.

emit: reconcile changes with the refactoring to support timeless points

ab21596

The rework to use the base class ticking behavior conflicted with the concurrent change to not add timestamps to the -points argument, so reconcile these two changes.

demmer force-pushed the adapters-are-not-procs branch from 01d09ac to ab21596 Compare January 29, 2016 19:55

davidvgalbraith reviewed Jan 30, 2016
View reviewed changes

demmer added 2 commits January 29, 2016 16:28

file: reorder promise handlers as per PR feedback

ba76a11

write: fix error due to unimplemented eof

7056250

demmer added a commit that referenced this pull request Jan 30, 2016

Merge pull request #248 from juttle/adapters-are-not-procs

e9f34a3

new adapter read and write api

demmer merged commit e9f34a3 into master Jan 30, 2016

demmer deleted the adapters-are-not-procs branch January 30, 2016 01:05

demmer removed the in progress label Jan 30, 2016

This was referenced Feb 1, 2016

port to the new adapter read / write API juttle/juttle-influx-adapter#43

Closed

Document the new adapter API #304

Closed

Refactor ticking for sources to a base class. #164

Closed

adapters should not directly inherit from proc #241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new adapter read and write api #248

new adapter read and write api #248

demmer commented Jan 22, 2016

demmer commented Jan 22, 2016

davidvgalbraith Jan 24, 2016

demmer Jan 25, 2016

demmer commented Jan 25, 2016

davidvgalbraith Jan 25, 2016

demmer Jan 25, 2016

davidvgalbraith commented Jan 25, 2016

davidvgalbraith Jan 25, 2016

davidvgalbraith Jan 27, 2016

davidvgalbraith Jan 27, 2016

demmer Jan 27, 2016

davidvgalbraith Jan 27, 2016

demmer Jan 27, 2016

demmer commented Jan 29, 2016

davidvgalbraith Jan 30, 2016

demmer Jan 30, 2016

davidvgalbraith commented Jan 30, 2016

demmer commented Jan 30, 2016

rlgomes commented Jan 30, 2016

new adapter read and write api #248

new adapter read and write api #248

Conversation

demmer commented Jan 22, 2016

demmer commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demmer commented Jan 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidvgalbraith commented Jan 25, 2016

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

demmer commented Jan 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidvgalbraith commented Jan 30, 2016

demmer commented Jan 30, 2016

rlgomes commented Jan 30, 2016