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.

Sign up for GitHub

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

Jump to bottom

Remove batchable datastore support #683

Closed
shahzadlone opened this issue Jul 28, 2022 · 5 comments · Fixed by #940
Closed

Remove batchable datastore support #683

shahzadlone opened this issue Jul 28, 2022 · 5 comments · Fixed by #940
Assignees
@fredcarle
Labels
deprecate Indicates something is deprecated. priority/low refactor This issue specific to or requires *notable* refactoring of existing codebases and components

Comments

@shahzadlone
Copy link
Member

Deprecate DEFRA_MAP environment variable as part of v0.4.

The following fails currently:

DEFRA_MAP=true go test -race -shuffle=on ./...
@shahzadlone shahzadlone added refactor This issue specific to or requires *notable* refactoring of existing codebases and components deprecate Indicates something is deprecated. labels Jul 28, 2022
@shahzadlone shahzadlone added this to the DefraDB v0.4 milestone Jul 28, 2022
@shahzadlone shahzadlone mentioned this issue Jul 28, 2022
Merged
9 tasks
@orpheuslummis
Copy link
Contributor

For reference, including discord discussion by John and Andy on this:

John: we can deprecate it. to prob run with BADGER_FILE and BADGER_MEMORY. just not MAP

Andy: Without defra map we won't be testing the batchable shim, I would be much much comfier if we only remove defra_map if we also drop support for non-transactional ipfs datastores (and remove related code in the same pr that removes defra_map)
As the above point is quite important I think, and defra_map has caught bugs that would otherwise have been missed

John: Yes, this is what I meant by deprecated. It's not worth all the shim magic I think. Unless we can identify an explicit example of a data store we'd like to use/explore that didn't have transactions.

@orpheuslummis
Copy link
Contributor

Currently the default for the DB integration test is run on map and badger in memory

@AndrewSisley
Copy link
Contributor

Update: CI runs badger file and badger IM

@AndrewSisley AndrewSisley changed the title Deprecate DEFRA_MAP in v0.4 Remove batchable datastore support Sep 23, 2022
@AndrewSisley AndrewSisley self-assigned this Sep 23, 2022
@AndrewSisley
Copy link
Contributor

Cannot remove batchable support atm as node depends on ipfslite which requires a Batchable datastore. This looks like this may no longer be a problem after #739

Marking this as blocked until #739 is merged, at which point this can be looked at again

There is a wip branch for this task here: https://github.com/sourcenetwork/defradb/pull/new/sisley/refactor/I683-rm-batchable

@AndrewSisley AndrewSisley removed their assignment Sep 23, 2022
@fredcarle fredcarle mentioned this issue Oct 16, 2022
Merged
4 tasks
@orpheuslummis orpheuslummis mentioned this issue Oct 17, 2022
Merged
4 tasks
fredcarle added a commit that referenced this issue Oct 18, 2022
@fredcarle
chore: Remove ipfslite dependency (#739)
a82ae40 
Relevant issue(s)
Resolves #738

Description
This PR removes our direct dependency on ipfslite. It is still an indirect dependency simply because it's used by an other package we import. Not from a function that we use but from a test of that package.

Note that some commented code will need to be uncommented once #683 is resolved. Although a ds.Batching implementing datastore needs to be used for the DHT within the net package, as long as badgerds.Datastore keeps the Batch method, the should have no problem.
@fredcarle fredcarle self-assigned this Nov 11, 2022
@fredcarle fredcarle mentioned this issue Nov 13, 2022
Merged
4 tasks
@jsimnz
Copy link
Member

jsimnz commented Dec 6, 2022

Blocked by #945

fredcarle added a commit that referenced this issue Dec 19, 2022
@fredcarle
refactor: Make rootstore require Batching and TxnDatastore (#940)
664d510 
Relevant issue(s)
Resolves #683

Description
This PR change the requirement for our supported datastore to implement both ds.Batching and ds.TxnDatastore. This means that the map datastore is no longer supported.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this issue Feb 23, 2024
@fredcarle
chore: Remove ipfslite dependency (sourcenetwork#739)
6256eeb 
Relevant issue(s)
Resolves sourcenetwork#738

Description
This PR removes our direct dependency on ipfslite. It is still an indirect dependency simply because it's used by an other package we import. Not from a function that we use but from a test of that package.

Note that some commented code will need to be uncommented once sourcenetwork#683 is resolved. Although a ds.Batching implementing datastore needs to be used for the DHT within the net package, as long as badgerds.Datastore keeps the Batch method, the should have no problem.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this issue Feb 23, 2024
@fredcarle
refactor: Make rootstore require Batching and TxnDatastore (sourcenet…
daa1319 
…work#940)

Relevant issue(s)
Resolves sourcenetwork#683

Description
This PR change the requirement for our supported datastore to implement both ds.Batching and ds.TxnDatastore. This means that the map datastore is no longer supported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fredcarle fredcarle

Labels
deprecate Indicates something is deprecated. priority/low refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: Make rootstore require Batching and TxnDatastore sourcenetwork/defradb
5 participants
@orpheuslummis @jsimnz @fredcarle @shahzadlone @AndrewSisley