Skip to content
This repository was archived by the owner on Apr 17, 2023. It is now read-only.

Conversation

@david-martin
Copy link
Member

@david-martin david-martin commented Jul 11, 2017

@wei-lee @aidenkeating Would you mind reviewing this change?
The mongodb-queue dependency has been updated to include a change for ttl support chilts/mongodb-queue#22

@david-martin david-martin force-pushed the add-mongodb-queue-with-ttl branch from 0dfae15 to 10a7068 Compare July 11, 2017 13:30
if (!existingIndex) {
needCreate = true;
return callback(null, false);
} else if (existingIndex.expireAfterSeconds !== self.queueTTL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does self.queueTTL exist anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Good spot. this might be related to the failing tests.
This should probably be this.queueOptions.ttl

david-martin added a commit to feedhenry/fh-mbaas-api that referenced this pull request Jul 11, 2017
This updates the version fo the fh-sync module to include a fix for the
ttl of queue messages.
See feedhenry/fh-sync#22 for more info.

The dependencies in package.json have also been updated to remove unused
dependencies.
These were discovered when looking at where the mongodb-queue module was
being required. It was originally required in fh-mbaas-api, but was
moved out with the sync implementation to the fh-sync module.
The majority of the unused modules were from fh-sync, with a few extra
ones discovered using this command:

```
dependency-check ./package.json --no-dev --unused
```

The npm-shrinkwrap.json file was regenerated as a result
@david-martin david-martin force-pushed the add-mongodb-queue-with-ttl branch from 10a7068 to bdd6660 Compare July 11, 2017 14:23
david-martin added a commit to feedhenry/fh-mbaas-api that referenced this pull request Jul 11, 2017
This updates the version fo the fh-sync module to include a fix for the
ttl of queue messages.
See feedhenry/fh-sync#22 for more info.

The dependencies in package.json have also been updated to remove unused
dependencies.
These were discovered when looking at where the mongodb-queue module was
being required. It was originally required in fh-mbaas-api, but was
moved out with the sync implementation to the fh-sync module.
The majority of the unused modules were from fh-sync, with a few extra
ones discovered using this command:

```
dependency-check ./package.json --no-dev --unused
```

The npm-shrinkwrap.json file was regenerated as a result
@david-martin david-martin force-pushed the add-mongodb-queue-with-ttl branch from bdd6660 to 0dd2f24 Compare July 11, 2017 14:47
david-martin added a commit to feedhenry/fh-mbaas-api that referenced this pull request Jul 11, 2017
This updates the version fo the fh-sync module to include a fix for the
ttl of queue messages.
See feedhenry/fh-sync#22 for more info.

The dependencies in package.json have also been updated to remove unused
dependencies.
These were discovered when looking at where the mongodb-queue module was
being required. It was originally required in fh-mbaas-api, but was
moved out with the sync implementation to the fh-sync module.
The majority of the unused modules were from fh-sync, with a few extra
ones discovered using this command:

```
dependency-check ./package.json --no-dev --unused
```

The npm-shrinkwrap.json file was regenerated as a result
@david-martin david-martin force-pushed the add-mongodb-queue-with-ttl branch from 0dd2f24 to 76ef90b Compare July 11, 2017 15:04
david-martin added a commit to feedhenry/fh-mbaas-api that referenced this pull request Jul 11, 2017
This updates the version fo the fh-sync module to include a fix for the
ttl of queue messages.
See feedhenry/fh-sync#22 for more info.

The dependencies in package.json have also been updated to remove unused
dependencies.
These were discovered when looking at where the mongodb-queue module was
being required. It was originally required in fh-mbaas-api, but was
moved out with the sync implementation to the fh-sync module.
The majority of the unused modules were from fh-sync, with a few extra
ones discovered using this command:

```
dependency-check ./package.json --no-dev --unused
```

The npm-shrinkwrap.json file was regenerated as a result
Copy link
Contributor

@aidenkeating aidenkeating left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Don't know if we want to wait until the mongodb-queue change is released before merging this and the fh-mbaas-api changes?

@david-martin
Copy link
Member Author

@aidenkeating I'm going to merge this now and revisit to reference the upstream version if/when the change is merged.

@david-martin david-martin merged commit 577fe7e into master Jul 11, 2017
@david-martin david-martin deleted the add-mongodb-queue-with-ttl branch July 11, 2017 16:22
david-martin added a commit to feedhenry/fh-mbaas-api that referenced this pull request Jul 11, 2017
This updates the version fo the fh-sync module to include a fix for the
ttl of queue messages.
See feedhenry/fh-sync#22 for more info.

The dependencies in package.json have also been updated to remove unused
dependencies.
These were discovered when looking at where the mongodb-queue module was
being required. It was originally required in fh-mbaas-api, but was
moved out with the sync implementation to the fh-sync module.
The majority of the unused modules were from fh-sync, with a few extra
ones discovered using this command:

```
dependency-check ./package.json --no-dev --unused
```

The npm-shrinkwrap.json file was regenerated as a result
@wtrocki
Copy link
Member

wtrocki commented Jul 21, 2017

@david-martin It looks like this change broke community version.
After migrating from 1.0.3 to 1.0.4 sync calls stopped responding and I'm getting timeouts.
Do I need to change something in client to support this?

@wtrocki wtrocki restored the add-mongodb-queue-with-ttl branch July 21, 2017 12:22
@wtrocki wtrocki deleted the add-mongodb-queue-with-ttl branch July 21, 2017 12:23
@wtrocki wtrocki mentioned this pull request Jul 21, 2017
@david-martin
Copy link
Member Author

@wtrocki Apologies for breaking something with this change.
That was certainly not the intent. The change is intended to be 100% backwards compatible with existing sync clients and servers. Looking at the change, it's hard to see how it would make sync requests timeout.
Perhaps the startup flow where it sets up indexes is stalling in some scenarios. A greenfield scenario seems to work OK given the test suites here and in the acceptance test repo. Also it is working in a producing scenario.

I wonder if existing indexes or records causes the problem when migrating? Could you get a reproducable test case or example?

@wtrocki
Copy link
Member

wtrocki commented Jul 21, 2017

@david-martin Things like that happening all the time so no worries, but I really stuck on debugging this so just wanted to point it out here. Code is breaking for Raincatcher but basically it will be possible to see the same problem when running example application in this repository. I'm going to use unreleased version for the moment and we can have chat about how this can be fixed without removing this change.

@wtrocki
Copy link
Member

wtrocki commented Jul 21, 2017

What I noticed is that initially sync works for 2-3 requests but then entire processing stops.
I add a lot of console logs for this but still cannot find place that is responsible for this behavior.

@aidenkeating
Copy link
Contributor

@david-martin @wtrocki I think this is because of this section here

function checkIndexTTL(indexInfo, callback) {
, it doesn't always invoke the callback. Will try to get around to a PR tonight.

@wtrocki
Copy link
Member

wtrocki commented Jul 21, 2017

Mentioned PR: #29

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants