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

refactor!: use exandra #71

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

Conversation

noaccOS
Copy link

@noaccOS noaccOS commented Aug 31, 2023

use exandra for queries instead of Xandra.Cluster

BREAKING CHANGE: XandraUtils module has been removed
BREAKING CHANGE: Interface.retrieve_interface_row now returns the interface struct

@noaccOS noaccOS force-pushed the exandra branch 3 times, most recently from b16c476 to 7e3c1f2 Compare April 30, 2024 16:58
@noaccOS noaccOS force-pushed the exandra branch 6 times, most recently from a2b76ff to ec4e13d Compare October 1, 2024 15:22
@noaccOS noaccOS changed the title draft: exandra refactor!: use exandra Oct 1, 2024
@noaccOS noaccOS force-pushed the exandra branch 2 times, most recently from 5c68385 to d75474b Compare October 1, 2024 15:48
@noaccOS noaccOS marked this pull request as ready for review October 1, 2024 15:59
Copy link
Collaborator

@Annopaolo Annopaolo left a comment

Choose a reason for hiding this comment

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

A part from some cosmetic comments, I think this change is very good and much needed. Bravo!

case Enum.to_list(page) do
[] ->
{:error, :property_not_set}
data_id = data_id(device_id, interface_id, endpoint_id, path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that in the Ecto lingo those are usually called clauses. In my opinion, it is a clearer name and it can also be used in the case of datastream interfaces (where the combination of device_id, interface_id, endpoint_id and path does not uniquely identify a single piece of data). So I would suggest something like

Suggested change
data_id = data_id(device_id, interface_id, endpoint_id, path)
fetch_clauses = fetch_clauses(device_id, interface_id, endpoint_id, path)

For reference, I'm looking at this part of the Ecto documentation: https://hexdocs.pm/ecto/Ecto.Repo.html#c:get_by/3

This applies in a number of points in the file, but I think you get the idea

alias Astarte.Core.Mapping
alias Astarte.DataAccess.Astarte.Realm
alias Astarte.DataAccess.Realms.Endpoint
alias Astarte.DataAccess.Repo
alias Astarte.DataAccess.XandraUtils
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this is now an unused alias

alias Astarte.Core.Mapping.Retention
alias Astarte.Core.Mapping.ValueType

@primary_key false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for consistency's sake, I'd suggest adding a @type t :: %__MODULE__{} here too, even if it won't be used.
The same holds for other similar files too, e.g. lib/astarte_data_access/realms/name.ex

Copy link
Author

Choose a reason for hiding this comment

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

I agree! I was also thinking of using TypedEctoSchema to do that automatically

mix.exs Outdated
@@ -71,7 +71,7 @@ defmodule Astarte.DataAccess.Mixfile do
# Run "mix help deps" to learn about dependencies.
defp deps do
[
{:xandra, "~> 0.11"},
{:exandra, github: "noaccos/exandra", ref: "push-ksolwvtqvqtr"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Damn, do your changes have been merged into exandra?

Copy link
Author

Choose a reason for hiding this comment

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

This was probably a mistake, yes. We're able to use exandra from main here (although we need custom a exandra in astarte-platform/astarte#1056 but that can be handled on the astarte side)

Signed-off-by: Francesco Noacco <[email protected]>
this also guarantees the port is always an integer

Signed-off-by: Francesco Noacco <[email protected]>
we need the updated functions which accept already casted values as well
as the database int values.

calling the functions without first updating astarte_core results in a
crash.

Signed-off-by: Francesco Noacco <[email protected]>
due to reworks in xandra, this option is now ignored and always enabled

Signed-off-by: Francesco Noacco <[email protected]>
@noaccOS noaccOS force-pushed the exandra branch 2 times, most recently from 9d6adf8 to 5e68d42 Compare February 3, 2025 16:14
add dependencies, repo and resources

Signed-off-by: Francesco Noacco <[email protected]>
BREAKING CHANGE: Interface.retrieve_interface_row now returns the struct
Signed-off-by: Francesco Noacco <[email protected]>
so as not to rewrite the tests, for now we still register the cluster
used by Exandra under the same name.
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