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

[WIP] Add offset query #150

Closed

Conversation

filosganga
Copy link
Contributor

This PR aim to fix the #95 adding the timestamp field to the journal allowing to query events by offset.

The data need a migration to populate the field from the ObjectId.

@filosganga
Copy link
Contributor Author

@scullxbones I have the migration on mongo shell script. What is the preferred way to deal with this changes? Doing a migration on the fly when the plugin is initialized? or offline?

We have this already in production, and we have run the migration after the service was already running.

Ideally, you should run a couple of time before being able to use the query with the offset to make sure all the events have the timestamp field populated.

We are very keen to see this merged, we all the modification you would like to make.

I will need to revert the version, but in the meantime would be good if you can give me an initial feedback.

@scullxbones
Copy link
Owner

Thanks @filosganga - will take a look at it!

In the past have done an online migration process, and there is also the tools subproject that could be a good place to collect migrations. Currently that's where the suffixed collection migration is run from.

@scullxbones
Copy link
Owner

I didn't much like the online forced data model upgrade process, but due to breaking backward compatibility it wasn't much of an option. Given that this can run quietly in the background, and is backward compatible, that would be much better. I think it would be good to provide a .js upgrade to run in the mongo console for those that prefer to migrate that way.

Haven't had a chance to look deeply but scanning through the code looked good so far. I'd prefer to not have to see code re-formatting edits next to code edits, when possible - but that was only in a few spots.

I hope to be able to dedicate some time to this tonight.

@filosganga
Copy link
Contributor Author

Thanks @scullxbones I will provide the migration js in the tools project and also remove our specific version.

@scullxbones
Copy link
Owner

Seems like tests are failing even locally - are you seeing the same?

@filosganga
Copy link
Contributor Author

@scullxbones I made a mistake in this PR. I will fix the tests as soon as I can.

@filosganga
Copy link
Contributor Author

Closed as I have no more time working at that

@filosganga filosganga closed this Oct 28, 2017
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