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

Adds support for multiple onupdate hook URLs #14

Merged
merged 51 commits into from
Mar 1, 2019
Merged

Conversation

solsson
Copy link
Contributor

@solsson solsson commented Feb 18, 2019

Note however that build-contract doesn't really verify this at the moment. You have to manually check logs for onupdate-logging.

The good news is that build-contract does pass, sometimes, now. See #3.

Once the nodejs based test is re-enabled we'll support validation of that url at least. See #5.

@solsson solsson mentioned this pull request Feb 18, 2019
@solsson
Copy link
Contributor Author

solsson commented Feb 24, 2019

Getting timeouts when running build-contract or compose:

example-nodejs-client_1  |     Timeout update on publish
example-nodejs-client_1  | 
example-nodejs-client_1  |       23 | 
example-nodejs-client_1  |       24 |     getUpdatePromise = optionalDescription => new Promise((resolve, reject) => {
example-nodejs-client_1  |     > 25 |       const timeoutId = setTimeout(() => reject(new Error('Timeout ' + optionalDescription)), 3000);
example-nodejs-client_1  |          |                                                 ^
example-nodejs-client_1  |       26 |       events.once('onupdate', body => {
example-nodejs-client_1  |       27 |         resolve(body);
example-nodejs-client_1  |       28 |         clearInterval(timeoutId);
example-nodejs-client_1  | 
example-nodejs-client_1  |       at setTimeout (onupdate-flow.spec.js:25:49)
example-nodejs-client_1  |       at Timeout.callback [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:678:19)

Maybe the internal name resolution doesn't work when the nodejs test isn't in `depends_on´ but if I add it there we'd get a circular dependency. We'd have to use standalone service that logs onupdate requests, with http based access for assertions.

@solsson
Copy link
Contributor Author

solsson commented Feb 27, 2019

@atamon I've pushed yolean/kafka-keyvalue@sha256:69f319da481ca3dd4755d919ad57d9169237de9654f9523987cff885bb351c00 from dd3b37d and verified that repeated builds ('./build-and-push.sh` but ignore the build-contract failure) produces the same sha - though probably only on my machine.

we've figured out how the json layout can be used with bunyan,
i.e. machine friendly logs can be piped to something that formats them nicely
ideally the same way we do with node.js services.

This layout tries to align message start vertically.
Also avoids location information lookup.
Kafka Streams thread names are really long.
Instead of placing them last we could truncate them
and not print full logger names.

I'd say this closes #4.
@solsson
Copy link
Contributor Author

solsson commented Feb 28, 2019

@atamon I've pushed sha256:ea36ae32c81171ab453651c05f5e76b7eb3e69376df1cdce5f7f51d3ecec981a with improved logging, in particular human-readability and including URLs and offsets for failed onupdate requests. I've also bumped kafka libs to 2.1.1.

@solsson solsson merged commit dedce3a into master Mar 1, 2019
@solsson
Copy link
Contributor Author

solsson commented Mar 1, 2019

build-contract passes now (though sometimes not when running dockerized as in ./build-and-push.sh so I've pushed :latest now from master: yolean/kafka-keyvalue@sha256:06c9b5e9f0d389d8973d827179a3d405b7ad99a8b1beb89c0cafa2699061220c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants