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

Reduce initial data request #4586

Merged
merged 30 commits into from
Oct 2, 2020

Conversation

chimp1984
Copy link
Contributor

@chimp1984 chimp1984 commented Oct 1, 2020

This is a clean version of #4519 which was based on #4233 from @freimair.
I decided to remove the git commit history and make new clean commits as it was too messy before and not easy to review.

Now the commits are clearly separated between pure refactoring, small not complex changes and the feature implementation.
It is basically only 2 commits which carry the main feature implementation. I hope that makes it easier to review and to get it merged.

In the last commit I deactivated the usage if the feature as we should deply it together with #4577 which is not ready yet and which will require some deployment planning and coordination. By reverting that last commit the feature gets activated.

By having the code base merged but not activated we avoid that the code base gets into problems with other large code change PRs which are pending but avoid any serious risk as it is a not executed code path.
Also we gain the benefit to have the version support deployed in case we decide to release the actiavted version in a later release.

@chimp1984 chimp1984 changed the title Reduce initial date request Reduce initial data request Oct 1, 2020
@bisq-network bisq-network deleted a comment from chimp1984 Oct 1, 2020
@ghubstan
Copy link
Contributor

ghubstan commented Oct 1, 2020

I take back the comment about extra white space -- I see it is intended and why.

utACK

@chimp1984 chimp1984 mentioned this pull request Oct 2, 2020
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

6f5bfde seems not necessary. If this code is activated but there is no historical data resource file it should work like does now.

Some changes suggested, but the protobuf index thing should be fixed.

…as it was the version used in master as well. We cannot change that without breaking compatibility.
@chimp1984
Copy link
Contributor Author

6f5bfde seems not necessary. If this code is activated but there is no historical data resource file it should work like does now.

Some changes suggested, but the protobuf index thing should be fixed.

We do not expect a resource file without version number, so we could not deploy the updated trade stats resources if we use the new store.

chimp1984 and others added 7 commits October 2, 2020 13:15
…calDataStoreService.java

Co-authored-by: sqrrm <sqrrm@users.noreply.github.com>
…calDataStoreService.java

Co-authored-by: sqrrm <sqrrm@users.noreply.github.com>
…calDataStoreService.java

Co-authored-by: sqrrm <sqrrm@users.noreply.github.com>
Co-authored-by: sqrrm <sqrrm@users.noreply.github.com>
Co-authored-by: sqrrm <sqrrm@users.noreply.github.com>
@chimp1984
Copy link
Contributor Author

@sqrrm thanks for the review. Added some minor commit and applied your suggestions.

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

ACK

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.

3 participants