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.

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

New version of Python API based on decorators #1833

Merged
merged 1 commit into from
Jan 9, 2018
Merged

Conversation

nitbix
Copy link
Contributor

@nitbix nitbix commented Dec 19, 2017

This is a breaking API change, so I've made this speculative PR while I prepare all the changes to examples and documentation so that we can start discussing potential other changes to the API.

Copy link
Contributor

@aturley aturley left a comment

Choose a reason for hiding this comment

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

I think this is a good improvement over the current API.


`payload_length` is used to decode our message header to determine how long the payload is going to be. In this case, we are relying on the Python `struct` package to decode the bytes. If you aren't familiar with `struct`, you can check out [the documentation](https://docs.python.org/2/library/struct.html) to learn more. Remember, when using `struct`, don't forget to import it!
`fmt` is used internally to decode our message header to determine how long the payload is going to be. We rely on the Python `struct` package to decode the bytes. If you aren't familiar with `struct`, you can check out [the documentation](https://docs.python.org/2/library/struct.html) to learn more. Remember, when using `struct`, don't forget to import it!
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be length_fmt, not fmt.

class TCPSourceConfig(object):
def __init__(self, host, port, decoder):
Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow decoders and encoders got switch around here and in TCPSinkConfig.

class TCPSourceConfig(object):
def __init__(self, host, port, decoder):
def __init__(self, host, port, encoder):
Copy link
Contributor

Choose a reason for hiding this comment

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

encoder -> decoder

self._host = host
self._port = port
self._decoder = decoder
self._encoder = encoder
Copy link
Contributor

Choose a reason for hiding this comment

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

encoder -> decoder


def to_tuple(self):
return ("tcp", self._host, self._port, self._decoder)
return ("tcp", self._host, self._port, self._encoder)
Copy link
Contributor

Choose a reason for hiding this comment

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

encoder -> decoder



class TCPSinkConfig(object):
def __init__(self, host, port, encoder):
def __init__(self, host, port, decoder):
Copy link
Contributor

Choose a reason for hiding this comment

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

decoder -> encoder

self._host = host
self._port = port
self._encoder = encoder
self._decoder = decoder
Copy link
Contributor

Choose a reason for hiding this comment

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

decoder -> encoder


def to_tuple(self):
return ("tcp", self._host, self._port, self._encoder)
return ("tcp", self._host, self._port, self._decoder)
Copy link
Contributor

Choose a reason for hiding this comment

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

decoder -> encoder

Copy link
Contributor

@aturley aturley left a comment

Choose a reason for hiding this comment

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

There's a few things that need to be fixed.

Copy link
Contributor

@aturley aturley left a comment

Choose a reason for hiding this comment

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

Looks ok now.

@nitbix nitbix force-pushed the new-python-api branch 2 times, most recently from 8c3b5c3 to 983f74f Compare January 2, 2018 21:31
@nitbix nitbix changed the title add non retro-compatible decorators New version of Python API based on decorators Jan 2, 2018
return ab.build()

@wallaroo.computation_multi(name="split into words")
Copy link
Contributor

Choose a reason for hiding this comment

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

You need another newline before this for PEP8 compatibility

@@ -79,13 +79,16 @@ def build(self):


def computation(name):
print 'computation', name
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need to be here does it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good catch! Thanks!

@nisanharamati
Copy link
Contributor

nisanharamati commented Jan 6, 2018

@SeanTAllen @nitbix @aturley
I have disabled the testing/performance/apps/python/market_spread test for the struct-based-serialization performance prototype of market_spread.
I created a separate issue to handle updating that application and re-enabling the test which can be done outside of this PR's scope (see #1922).

The Python API was using a ''class-based'' approach that was very
similar to the C++ API. This wasn't very pythonic, so this change adds
decorators in place of a lot of the boiler-plate classes and builders we
were using. This results in shorter application code, and less
responsibility on the developer to respect Wallaroo interfaces.
@aturley aturley merged commit 36296bc into master Jan 9, 2018
wallaroolabs-wally added a commit that referenced this pull request Jan 9, 2018
@aturley aturley deleted the new-python-api branch January 9, 2018 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants