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

uplink: remove dependency to storage/ #3720

Merged
merged 2 commits into from
Dec 10, 2019

Conversation

egonelbre
Copy link
Member

@egonelbre egonelbre commented Dec 10, 2019

Remove dependency to "storj/storage". This helps to split the repositories.

Please describe the tests:

  • Test 1:
  • Test 2:

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

  • NEW: Are there any Satellite database migrations? Are they forwards and backwards compatible?
  • Does the PR describe what changes are being made?
  • Does the PR describe why the changes are being made?
  • Does the code follow our style guide?
  • Does the code follow our testing guide?
  • Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
  • Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
  • Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
  • Does any documentation need updating?
  • Do the database access patterns make sense?

@cla-bot cla-bot bot added the cla-signed label Dec 10, 2019
@egonelbre egonelbre marked this pull request as ready for review December 10, 2019 16:21
@egonelbre egonelbre requested review from a team and calebcase December 10, 2019 16:21
@egonelbre egonelbre added Request Code Review Code review requested Reviewer Can Merge If all checks have passed, non-owner can merge PR labels Dec 10, 2019
@ghost ghost requested review from JessicaGreben and Qweder93 and removed request for a team December 10, 2019 16:22
Copy link
Contributor

@stefanbenten stefanbenten left a comment

Choose a reason for hiding this comment

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

Small usability question, rest looks good.

@@ -49,10 +48,13 @@ func New(project *Project, metainfo *metainfo.Client, streams streams.Store, seg
}
}

const defaultLookupLimit = 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to pass this as an config value or pass it through on creation?

Copy link
Member Author

Choose a reason for hiding this comment

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

uplink needs to request this from the satellite, because this may vary from satellite-to-satellite.

@egonelbre egonelbre merged commit 218f436 into master Dec 10, 2019
@egonelbre egonelbre deleted the ee/uplink-remove-storage-dependency branch December 10, 2019 20:07
bryanchriswhite added a commit that referenced this pull request Dec 12, 2019
* storj/master: (436 commits)
  satellite/console: project usage limits api (#3702)
  web/storagenode: charts titles aligned
  cmd/segment-reaper: test processSegment for single project
  web/satellite: add tracking event for segment.io (#3641)
  cmd/storj-sim: make initial provisioning nicer
  adding Igor Gaidaenko our new team member! (#3726)
  segment-reaper: fix for not analyzing last project in detect command
  satellite/metainfo: Return error misusing func
  cmd/storagenode-updater, pkg/process: Fix logging timestamp
  storagenodedb: reenable utccheck in tests
  storagenode/trust: source entry cache
  storagenode/trust: rule and excluders
  projectLimit error message changed (#3718)
  pkg/rpc/rpcpool: add idle expiration to connections
  storage/testsuite: pass ctx in to bulk setup methods
  satellite/metainfo: don't leak error implementation detail (#3722)
  uplink: remove dependency to storage/ (#3720)
  jenkins: run storj-sim integration tests with cockraochdb  (#3723)
  private/dbutil/cockroachutil: keep crdb connstr for tests
  replace planet.Start in tests with planet.Run
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed Request Code Review Code review requested Reviewer Can Merge If all checks have passed, non-owner can merge PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants