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

Revised implementation of DDP resumption #11845

Closed
wants to merge 16 commits into from

Conversation

vlasky
Copy link
Contributor

@vlasky vlasky commented Jan 12, 2022

Changes to DDP resumption code in response to reviewer comments on znewsham's PR #11559

  • Constant MAX_QUEUE_LENGTH has been replaced with option maxMessageQueueLength
  • Constant DISCONNECT_GRACE_PERIOD has been replaced with option disconnectGracePeriod
  • Use of _.each() replaced with Array forEach()
  • isPingPong replaced with descriptive name ignoredMsgsForSessionOutOfDateCheck

znewsham and others added 6 commits July 26, 2021 17:39
Constant MAX_QUEUE_LENGTH has been replaced with option maxMessageQueueLength
Constant DISCONNECT_GRACE_PERIOD has been replaced with option disconnectGracePeriod
Use of _.each() replaced with Array forEach()
isPingPong replaced with descriptive name ignoredMsgsForSessionOutOfDateCheck
@vlasky vlasky changed the base branch from devel to release-2.5.2 January 12, 2022 04:34
@denihs
Copy link
Contributor

denihs commented Feb 3, 2022

Hi @vlasky, this PR has tests failing. Could you address those?

Also, would you be able to add one test or two for your new code?

@filipenevola filipenevola added the pending-tests Tests are not passing, stuck or we need new tests label Feb 10, 2022
@denihs
Copy link
Contributor

denihs commented Feb 10, 2022

Hi, we still have tests that are failing, and those tests seem to be related to the changes.

So I'm closing this PR until someone fixes the tests.

@denihs denihs closed this Feb 10, 2022
@StorytellerCZ StorytellerCZ self-assigned this Feb 11, 2022
@vlasky
Copy link
Contributor Author

vlasky commented Mar 23, 2022

I have merged the fixes made by @StorytellerCZ. Can this PR be reopened or should I create a new one?

@StorytellerCZ
Copy link
Collaborator

I'm going to push the re-open button and see if it picks up the new changes and runs the tests.

@denihs denihs added this to the Release 2.7.1 milestone Mar 28, 2022
@StorytellerCZ
Copy link
Collaborator

The error is annoying. Nothing obvious is coming up right now that would explain it, but I'm working on it.

@denihs denihs removed this from the Release 2.7.1 milestone Mar 29, 2022
@vlasky
Copy link
Contributor Author

vlasky commented Aug 17, 2022

Hiya @StorytellerCZ, what needs to be addressed to allow this to be merged?

@Grubba27
Copy link
Contributor

hello there! @vlasky, the changes we made in 2.8 probably will make this Travis CI pass now.

@StorytellerCZ StorytellerCZ changed the base branch from devel to release-2.7.4 August 23, 2022 14:20
@StorytellerCZ
Copy link
Collaborator

@vlasky, can you merge in the release-2.7.4 branch into yours? I have already changed the target branch.

@vlasky
Copy link
Contributor Author

vlasky commented Aug 26, 2022

@StorytellerCZ unfortunately Travis CI failed again :-(

@fredmaiaarantes fredmaiaarantes added this to the Release 2.9 milestone Aug 28, 2022
@vlasky vlasky changed the base branch from release-2.7.4 to release-2.8.1 October 27, 2022 14:39
@vlasky
Copy link
Contributor Author

vlasky commented Oct 27, 2022

@StorytellerCZ I have merged release-2.8.1 into my ddp-resumption branch.

@StorytellerCZ
Copy link
Collaborator

@vlasky thank you! I'm trying to figure out why the Travis tests are failing.

@denihs denihs modified the milestones: Release 2.9, Release 2.10 Nov 18, 2022
@vlasky vlasky changed the base branch from release-2.8.1 to release-2.9.1 January 9, 2023 07:28
@vlasky
Copy link
Contributor Author

vlasky commented Jan 9, 2023

@StorytellerCZ I have merged release-2.9.1 into my ddp-resumption branch.

@StorytellerCZ
Copy link
Collaborator

Hi @vlasky, I think I will make another PR from my own branch here in the Meteor repo, so that it is easier for us to access.

@vlasky
Copy link
Contributor Author

vlasky commented Jan 10, 2023

Hi @vlasky, I think I will make another PR from my own branch here in the Meteor repo, so that it is easier for us to access.

Yes thanks anything that will help get this deployed sooner!

@StorytellerCZ
Copy link
Collaborator

Superseded by #12436

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-tests Tests are not passing, stuck or we need new tests Project:DDP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants