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.

Sign up for GitHub

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

Jump to bottom

Idempotent Rest Publishing #786

Merged
merged 22 commits into from
Apr 24, 2019
Merged

Idempotent Rest Publishing #786

merged 22 commits into from
Apr 24, 2019

Conversation

ricardopereira
Copy link
Contributor

@ricardopereira
Copy link
Contributor Author

ricardopereira commented Aug 30, 2018 •
edited
Loading

RSL1k4 and RSL1k5 aren't passing, maybe because I should run the test suite with dev environment, I think.

⚠️ Update:
Now it's running in idempotent-dev but we shouldn't merge with that (04ed83c)

Update 2:
Idempotent Publishing is now enabled in sandbox.

@ricardopereira ricardopereira force-pushed the idempotent-rest-publishing branch from 78946f6 to 7f38002 Compare August 30, 2018 13:37
@ricardopereira
Copy link
Contributor Author

@paddybyers like you told me, RTN10c is failing in dev. Did you found anything suspicious?

@paddybyers
Copy link
Member

Can you update and retry this pls? dev should now implement the spec fully

@ricardopereira
Copy link
Contributor Author

@paddybyers still happens. Sorry.

Test Suite 'RealtimeClientConnection' started at 2018-09-06 09:39:16.419
Test Case '-[AblySpec.RealtimeClientConnection Connection__serial__should_have_last_known_connection_serial_from_restored_connection]' started.
/Users/ricardopereira/Repositories/Whitesmith/ably-ios/Spec/RealtimeClientConnection.swift:1441: error: -[AblySpec.RealtimeClientConnection Connection__serial__should_have_last_known_connection_serial_from_restored_connection] : Waited more than 10.0 seconds

Test Case '-[AblySpec.RealtimeClientConnection Connection__serial__should_have_last_known_connection_serial_from_restored_connection]' failed (11.735 seconds).
Test Suite 'RealtimeClientConnection' failed at 2018-09-06 09:39:28.156.

@paddybyers
Copy link
Member

still happens. Sorry.

@ricardopereira I can't find the log you created of the protocol traffic when this test runs. Can you re-send the link pls?

@ricardopereira
Copy link
Contributor Author

@paddybyers Sure. https://gist.github.com/ricardopereira/642d8ec61958f51b21f836a58b72a85f

@ricardopereira ricardopereira force-pushed the idempotent-rest-publishing branch from 17c6576 to b8ca851 Compare October 3, 2018 15:10
@ricardopereira
Copy link
Contributor Author

@paddybyers Should I still use the dev environment?

@mattheworiordan
Copy link
Member

@ricardopereira we will provide a stable env for you to use shortly. @paddybyers please confirm when available.

@ricardopereira ricardopereira force-pushed the idempotent-rest-publishing branch from 37ab6f3 to 04ed83c Compare October 7, 2018 11:05
@ricardopereira
Copy link
Contributor Author

ricardopereira commented Oct 7, 2018 •
edited
Loading

@paddybyers like you told me, RTN10c is failing in dev. Did you found anything suspicious?

So after #800, RTN10c in idempotent-dev environment is still failing. Plus, a new test is failing in that environment: https://github.com/ably/ably-ios/blob/6a755c6fd52ea26be7d89043333968f86f558b9b/Spec/PushAdmin.swift#L232

Test suite failures:

✗ publish__should_publish_successfully, Waited more than 10.0 seconds
✗ Connection__serial__should_have_last_known_connection_serial_from_restored_connection, Waited more than 10.0 seconds

@ricardopereira ricardopereira mentioned this pull request Nov 30, 2018
Closed
@mattheworiordan
Copy link
Member

@ricardopereira I can't review for over a week. @paddybyers can you approve when you review? @ricardopereira you will need to make sure this PR disables default idempotency in 1.1 though, it's only enabled by default in 1.2+

paddybyers
paddybyers requested changes Jan 3, 2019
View reviewed changes
Copy link
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

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

This needs the 1.1 -> 1.2 change, but otherwise LGTM.

Source/ARTClientOptions.m Outdated Show resolved Hide resolved
Spec/RestClientChannel.swift Outdated Show resolved Hide resolved
paddybyers
paddybyers approved these changes Jan 4, 2019
View reviewed changes
Copy link
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@ricardopereira ricardopereira force-pushed the idempotent-rest-publishing branch from 7461c58 to dfd79c1 Compare January 4, 2019 14:28
@ricardopereira
Copy link
Contributor Author

@paddybyers Connection__serial__should_have_last_known_connection_serial_from_restored_connection, Waited more than 10.0 seconds is still failing. Should I mark it has pending and merge this?

@ricardopereira
Revert "idempotent-dev env"
c9a9c36 
This reverts commit 04ed83c.
@ricardopereira ricardopereira merged commit 3b8c581 into develop Apr 24, 2019
@ricardopereira ricardopereira deleted the idempotent-rest-publishing branch April 24, 2019 14:31
maratal pushed a commit that referenced this pull request Jul 19, 2023
@paddybyers
Push Activation State Machine: add requirement for registration valid…
bd92372 
…ation (#786)

* Push Activation State Machine: add requirement for registration validation

* Push LocalDevice: add requirements for clientId immutability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paddybyers paddybyers paddybyers approved these changes

@mattheworiordan mattheworiordan Awaiting requested review from mattheworiordan

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ricardopereira @paddybyers @mattheworiordan