Skip to content

feat: Add support for batch delete #78

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

Open
munna0908 wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: Add support for batch delete #78

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
237f2d2
add support for batch deletes
munna0908 Mar 27, 2025
39c400a
change method signatures
munna0908 Mar 27, 2025
0beff39
code cleanup
munna0908 Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions datastore/leveldb/leveldbds.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,31 +34,39 @@ method delete*(self: LevelDbDatastore, key: Key): Future[?!void] {.async.} =
except LevelDbException as e:
return failure("LevelDbDatastore.delete exception: " & e.msg)

method delete*(self: LevelDbDatastore, keys: seq[Key]): Future[?!void] {.async.} =
for key in keys:
if err =? (await self.delete(key)).errorOption:
return failure(err.msg)
return success()
method delete*(self: LevelDbDatastore, keys: seq[Key]): Future[
?!void] {.async.} =
Copy link
Contributor

@emizzle emizzle Mar 28, 2025

Choose a reason for hiding this comment

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

🟡 This comment is likely beyond the scope of this PR, but we need to address this at some point (probably in a separate PR). I know you were amending what was already there, so definitely not something you have done. This is more for general information.

We should always try to annotate async pragmas with raises to inform the compiler and callers of potential exceptions that can be raised. This is useful so that callers can distinguish between exceptions being raised in a try/except block and potentially perform different operations depending on which exception was raised, eg

try:
  await key.read()
  await key.delete()
  key.removeFromListOfKeysToDelete()
except ReadError as e:
  # already doesn't exist, remove from deletion list
  key.removeFromListOfKeysToDelete()
except DeleteError as e
  # failed to delete, keep on deletion list, but may keep happening
  error "failed to delete", error = e.msg

is much more informative than:

try:
  await key.read()
  await key.delete()
  key.removeFromListOfKeysToDelete()
except CatchableError as e:
  # here we don't know which module raised the exception, and can only tell by inspecting the error message
  if "read failed" in e.msg:
    # already doesn't exist, remove from deletion list
    key.removeFromListOfKeysToDelete()
  elif "delete failed" in e.msg:
    # failed to delete, keep on deletion list, but may keep happening
    error "failed to delete", error = e.msg
  else:
    error "unknown error", error = e.msg

However, the operations in this module use return failure("some string") which returns a CatchableError instead of a module-specific and operation-specific exception, which means that the signatures in this module would simply have {.async: (raises: [CatchableError]).}. This does not give the caller much flexibility, where their only choice is wrap the calls in a try/except CatchableError and distinguish which exception was raised based on the error message.

So, we should be aiming to have module-specific errors as a base, and operation-specific errors as the raised exception, ie

type
  LevelDbDatastoreError = object of DatastoreError
  LevelDbReadError = object of LevelDbDatastoreError
  LevelDbDeleteError = object of LevelDbDatastoreError

method delete*(self: LevelDbDatastore, key: Key): Future[?!void] {.async: (raises: [LevelDbDeleteError]).} =
  try:
    #...
  except LevelDbException as e: 
    return failure newException(LevelDbDeleteError, "LevelDbDatastore.delete batch exception: " & e.msg, e)

This allows callers the flexibility to handle all exceptions from the module with eg try/except LevelDbDatastoreError, or operation-specific exceptions, eg try/except LevelDbDeleteError.

Copy link
Author

@munna0908 munna0908 Mar 28, 2025

Choose a reason for hiding this comment

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

@emizzle We have an active PR, Will accommodate your suggestions as part of that PR

Copy link
Contributor

@emizzle emizzle Mar 28, 2025

Choose a reason for hiding this comment

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

Sorry, I misspoke a bit earlier. Since we're returning a Result which captures the exceptions, the raises pragma should only raise CancelledError, but the remainder of what I described should work.

try:
let b = newBatch()
for key in keys:
b.delete($key)
self.db.write(b)
return success()
except LevelDbException as e:
return failure("LevelDbDatastore.delete batch exception: " & e.msg)

method get*(self: LevelDbDatastore, key: Key): Future[?!seq[byte]] {.async.} =
try:
let str = self.db.get($key)
if not str.isSome:
return failure(newException(DatastoreKeyNotFound, "LevelDbDatastore.get: key not found " & $key))
return failure(newException(DatastoreKeyNotFound,
"LevelDbDatastore.get: key not found " & $key))
let bytes = str.get().toBytes()
return success(bytes)
except LevelDbException as e:
return failure("LevelDbDatastore.get exception: " & $e.msg)

method put*(self: LevelDbDatastore, key: Key, data: seq[byte]): Future[?!void] {.async.} =
method put*(self: LevelDbDatastore, key: Key, data: seq[byte]): Future[
?!void] {.async.} =
try:
let str = string.fromBytes(data)
self.db.put($key, str)
return success()
except LevelDbException as e:
return failure("LevelDbDatastore.put exception: " & $e.msg)

method put*(self: LevelDbDatastore, batch: seq[BatchEntry]): Future[?!void] {.async.} =
method put*(self: LevelDbDatastore, batch: seq[BatchEntry]): Future[
?!void] {.async.} =
try:
let b = newBatch()
for entry in batch:
Expand Down Expand Up @@ -100,7 +108,8 @@ method query*(

proc next(): Future[?!QueryResponse] {.async.} =
if iter.finished:
return failure(newException(QueryEndedError, "Calling next on a finished query!"))
return failure(newException(QueryEndedError,
"Calling next on a finished query!"))

try:
let (keyStr, valueStr) = dbIter.next()
Expand Down