Skip to content

Conversation

@carlosms
Copy link
Contributor

Related to #30.
Done on top of #32, only the last commit is relevant 5249912.

Since the issues, PRs and the repository are all committed at the same time, we can remove the FindOne checks for issues and PRs because we already know the repository was not found.

After testing this change the download times for src-d did not change, it's still around 15m. That's why I'm submitting this as a separate PR, in case we consider this complicates the code and prefer to just merge #32.

carlosms added 6 commits June 17, 2019 17:47
Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>
Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>
Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>
Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>
Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>
Since the issues, PRs and the reposity are all commited at
the same time, we can remove the FindOne checks for issues
and because we already know the repository was not found

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>
@carlosms carlosms requested a review from a team June 19, 2019 09:09
@carlosms carlosms mentioned this pull request Jun 19, 2019
@smacker
Copy link
Contributor

smacker commented Jun 19, 2019

If it doesn't improve performance I don't think we should merge it.
The problem not really with the code but in changing something without proper consideration of tradeoffs and effect on future development.
In my opinion, after the release, we need to come back to a bigger picture with support for sync not only import, importing all entities not only PR/Issues. And it might affect the way we work with DB.
If we find this approach is actually correct we can always rebase. There shouldn't be many conflicts.

@smacker
Copy link
Contributor

smacker commented Jun 25, 2019

One of my concerns is if we remove FindOne we won't be able to do re-sync without dropping everything and re-importing completely from scratch. Let's move it back to TODO while we don't have DD/Roadmap for ghsync.

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