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

Add collection modifying #18

Merged
merged 18 commits into from
Jun 24, 2020

Conversation

arn-the-long-beard
Copy link
Contributor

@arn-the-long-beard arn-the-long-beard commented Jun 22, 2020

Hello ! Let's implement from the Arangodb documentation -> Modifying a Collection

NB : In this todo list, the unit test goes together with the implementation. So an implementation is considered done only if the unit test is also done :).

  • load collection
  • unload collection
  • load indexes into memory
  • change properties of a collection
  • rename collection
  • rotate journal
  • recalculate count of a collection

Let's go !

@arn-the-long-beard arn-the-long-beard changed the title Add collection Modifying Add collection modifying Jun 22, 2020
@arn-the-long-beard
Copy link
Contributor Author

I just have seen that I did not use CollectionType for type ( r#type) that we have on our structures.
Should I update them to have Collectiontype instead of u16 ?

@fMeow
Copy link
Owner

fMeow commented Jun 22, 2020

Yes. You should use CollectionType.

@arn-the-long-beard
Copy link
Contributor Author

Okay ! Thank you for your feedback !

@arn-the-long-beard
Copy link
Contributor Author

I updated with collection type :)
What do you think about collection::load implementation ?

pub async fn load(&self) {
unimplemented!()
pub async fn load(&self, count: bool) -> Result<CollectionLoad, ClientError> {
let url = self.base_url.join(&format!("load")).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Here Url::join accepts a &str, and "load" is already a &str. So we don't need to allocate memory to store "load" as a String and convert it to &str.

The following code suffices:

let url = self.base_url.join("load").unwrap();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ho yes ! True ! Thank you, I' ll make the update.

@fMeow
Copy link
Owner

fMeow commented Jun 22, 2020

I updated with collection type :)
What do you think about collection::load implementation ?

Quite good I think.

@arn-the-long-beard
Copy link
Contributor Author

Ok, cool ! The json stuff from serde is just awesome !

@arn-the-long-beard
Copy link
Contributor Author

1- There are differences between ArangoDB 3.6 and 3.7 for update properties. Should I implement the schema validation used in 3.7 ?

2 - I did not implemented the journalSize because the official documentation states the following :

APIs that return collection properties or figures will return slightly different attributes for the RocksDB engine than for the MMFiles engine. For example, the attributes journalSize, doCompact, indexBuckets and isVolatile are present in the MMFiles engine but not in the RocksDB engine.

The MMFiles storage engine is deprecated starting with version 3.6.0 and it will be removed in a future release. To change your MMFiles storage engine deployment to RocksDB, see: Switch storage engine

@arn-the-long-beard
Copy link
Contributor Author

arn-the-long-beard commented Jun 23, 2020

1 - The rename seems ok I think, but I feel some of structures to be redundant. Do you think it is fine to have structs that are pretty similar ? I know it is good in the way that the type is not a giant mess of Option used everywhere but several small structures. But in the other side, we do repeat a lot of code.

2 - It seems the Recalculate count of a collection has not been written in collection.rs.
Is it ok if I add it and implement it? :)

@fMeow
Copy link
Owner

fMeow commented Jun 23, 2020

1 - The rename seems ok I think, but I feel some of structures to be redundant. Do you think it is fine to have structs that are pretty similar ? I know it is good in the way that the type is not a giant mess of Option used everywhere but several small structures. But in the other side, we do repeat a lot of code.

2 - It seems the Recalculate count of a collection has not been written in collection.rs.
Is it ok if I add it and implement it? :)

  1. I perfer to find a way to abstract over similar structures. We can amass related fields in a struct and mark the struct as Option. But I'm not sure if it is a good idea.
  2. Sure. Thank you.

@fMeow
Copy link
Owner

fMeow commented Jun 23, 2020

1- There are differences between ArangoDB 3.6 and 3.7 for update properties. Should I implement the schema validation used in 3.7 ?

2 - I did not implemented the journalSize because the official documentation states the following :

APIs that return collection properties or figures will return slightly different attributes for the RocksDB engine than for the MMFiles engine. For example, the attributes journalSize, doCompact, indexBuckets and isVolatile are present in the MMFiles engine but not in the RocksDB engine.

The MMFiles storage engine is deprecated starting with version 3.6.0 and it will be removed in a future release. To change your MMFiles storage engine deployment to RocksDB, see: Switch storage engine

  1. You can implement this if you like. I don't know if specifying schema using version below 3.7 can cause error. I think ArangoDB server should just ignore this additional and unknown field.

  2. For breaking changes across versions or engines, I prefer to use a feature gate to switch between. There is a feature gate names mmfiles and another named rocksdb. But it's fine to leave it alone until somebody creates an issue on this.

@arn-the-long-beard
Copy link
Contributor Author

arn-the-long-beard commented Jun 24, 2020

Hey :) Thank you for your messages !

1 - Yes ! Nice to find abstraction. I am just not sure yet on how to do it. We can discuss it later on how to do it.
2 - Okay, I will add the recalculation.
3 - I do not have ArangoDB 3.7, so I will need to upgrade in order to make the unit test for it. I will think about it, or maybe I will leave a note in the code so someone could make it.

4 - Nice ! Let's say that the core focus now is on the main functionalities and we can work out details later on :)

@arn-the-long-beard
Copy link
Contributor Author

I will make later a PR for feature gate for versions switch and code improvement in collection.

@arn-the-long-beard arn-the-long-beard marked this pull request as ready for review June 24, 2020 08:28
@arn-the-long-beard
Copy link
Contributor Author

arn-the-long-beard commented Jun 24, 2020

Actually, I forgot to implement Rotate journal

This functionalities is not documented on 3.7 and is for the MMFiles engine

@arn-the-long-beard arn-the-long-beard marked this pull request as draft June 24, 2020 08:33
@arn-the-long-beard
Copy link
Contributor Author

arn-the-long-beard commented Jun 24, 2020

OMG, I just have understood how feature gate stuff works. It allows conditional compilation. That is freaking awesome and ergonomic. Rust rocks so hard lol.

We need somebody to try on mmfiles.

@arn-the-long-beard arn-the-long-beard marked this pull request as ready for review June 24, 2020 09:11
@fMeow fMeow changed the base branch from master to develop June 24, 2020 10:23
@fMeow
Copy link
Owner

fMeow commented Jun 24, 2020

Fine, I will do the migration works myself.

Thank you for your great works!!!

@fMeow fMeow merged commit 7ecb25e into fMeow:develop Jun 24, 2020
@fMeow
Copy link
Owner

fMeow commented Jun 24, 2020

OMG, I just have understood how feature gate stuff works. It allows conditional compilation. That is freaking awesome and ergonomic. Rust rocks so hard lol.

We need somebody to try on mmfiles.

That's why I use docker to set up ArangoDB in e2e test. With docker, we can quick setup an ArangoDB server with any verison and any configuration.

It's fine to just wait for someone to point out that this code errs. But if we want to make for sure before anyone jumps out, we can allocate a new job in CI and setup ArangoDB to mmfiles. I migrate CI to github workflow today, but that's as simple as travis CI. I regard it as a description about a pipeline of shell commands.

Maybe I should consider giving up docker compose, and use plain docker command instead.

@fMeow
Copy link
Owner

fMeow commented Jun 24, 2020

I have set up a job in github workflow using ArangoDB server with mmfiles engine. Let's see if the rotate_journal works.

@arn-the-long-beard
Copy link
Contributor Author

arn-the-long-beard commented Jun 24, 2020

Nice ! I used to play with Docker some time ago, so not sure . Need to try it out again before I can help with it.
Cool ! having test with ci. Hum it seems the unit test failed for some reason.

Deserialize normal Response: {"code": Number(404), "error": Bool(true), "errorMessage": String("collection or view not found"), "errorNum": Number(1203)}

@arn-the-long-beard arn-the-long-beard deleted the feature/collection-modifying branch June 24, 2020 12:00
@fMeow
Copy link
Owner

fMeow commented Jun 24, 2020

Yes, I am working on this.

@fMeow
Copy link
Owner

fMeow commented Jun 24, 2020

It seem that collection details in mmfiles and rocksdb are quite different.

I have fixed the error. Now it work like a charm (see workflow).

In the latest develop branch, I have merge similar fileds on collection into CollectionDetail and CollectionInfo.

@arn-the-long-beard
Copy link
Contributor Author

arn-the-long-beard commented Jun 24, 2020

Thank you ! I should have paid attention to it. Now i know how feature stuff works, I will be more careful to the differences between both storage engines.
It awesome how we can conditionally define props even inside a structure.

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.

None yet

2 participants