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

Alias system #583

Merged
merged 52 commits into from
Dec 3, 2018
Merged

Alias system #583

merged 52 commits into from
Dec 3, 2018

Conversation

NicolasMahe
Copy link
Member

@NicolasMahe NicolasMahe commented Nov 23, 2018

Fix #558 about re-using the same volumes.
Closes #607, closes #605 and closes #612

This PR introduce a new optional property called alias in mesg.yml.
The alias can be use instead of the generated service id in any API and command that accept a service ID.
If the alias is omitted, one is generated based on the service hash.
If the alias already exist, it get replace by the last deployed service.

There is no breaking change.

database/service_db.go Outdated Show resolved Hide resolved
database/service_db.go Outdated Show resolved Hide resolved
service/namespace.go Outdated Show resolved Hide resolved
database/service_db.go Outdated Show resolved Hide resolved
service/importer/assets/schema.json Outdated Show resolved Hide resolved
database/service_db.go Show resolved Hide resolved
@krhubert
Copy link
Contributor

It would be great if we can provide versioning along with alias.

@ilgooz
Copy link
Contributor

ilgooz commented Nov 25, 2018

It would be great if we can provide versioning along with alias.

What do you mean exactly? We still have the id of service which is its version. Do you've something like semver in your mind? Semver is much more readable than what we have now. Like docker containers we can have these short names: http-server:latest, http-server:3.8. But having long hash IDs might be required while running on blockchain to easily identify the author of a service by service's source code. I'm not sure, maybe @NicolasMahe & @antho1404 can share their opinions about this.

@antho1404
Copy link
Member

The idea with the versioning is to have a list of hashes for a specific alias instead of a single hash for a specific alias with by default the first one (or last one) is the latest deployment but we could go back in time with all the deployments and see for the same alias all the different versions updated.

It's not really versions like semver, it's more all the previous versions deployed. (Is it what you were thinking @krhubert ?)

This is something we might do later on but i really wanted to keep it simple for now

@mesg-bot
Copy link
Member

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/fixed-service-id-name-service-update-create-feature/110/4

database/service_db.go Show resolved Hide resolved
database/service_db.go Outdated Show resolved Hide resolved
@NicolasMahe
Copy link
Member Author

@ilgooz > It's a bit better to do atomic reads (with transactions) for Get & All as well.

Transaction are only useful to group multiple reads and writes into a single atomic transaction.
If it only writes, batch is enough.
If it only one read, no need for transaction.
If it multiple reads, transactions are useful.

For the All function, i don't think we need transaction because Iterator is returning a snapshot of the database. So concurrent modifications are not affecting the iterator.

database/service_db.go Outdated Show resolved Hide resolved
require.Equal(t, leveldb.ErrNotFound, err)
}

func TestServiceDBDeleteConcurrency(t *testing.T) {
Copy link
Member Author

@NicolasMahe NicolasMahe Dec 3, 2018

Choose a reason for hiding this comment

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

I add this test in order to make sure that Delete function behave correctly on concurrent deletion.
If you remove the transaction in Delete function, then this test fails.

To test, replace in Delete function tx, err := d.db.OpenTransaction() by tx = db.db and remove tx.Discard() and tx.Commit()

@NicolasMahe NicolasMahe requested review from krhubert, antho1404 and ilgooz and removed request for ilgooz December 3, 2018 05:35
@NicolasMahe
Copy link
Member Author

NicolasMahe commented Dec 3, 2018

@mesg-foundation/core please review. I add few tests and change some minor code.
Please don't commit anything on this PR

@NicolasMahe NicolasMahe requested review from antho1404 and removed request for antho1404 December 3, 2018 05:46
@ilgooz
Copy link
Contributor

ilgooz commented Dec 3, 2018

@ilgooz > It's a bit better to do atomic reads (with transactions) for Get & All as well.

Transaction are only useful to group multiple reads and writes into a single atomic transaction.
If it only writes, batch is enough.
If it only one read, no need for transaction.
If it multiple reads, transactions are useful.

For the All function, i don't think we need transaction because Iterator is returning a snapshot of the database. So concurrent modifications are not affecting the iterator.

Here there is multiple reads https://github.com/mesg-foundation/core/blob/64407a543a353286229e8ba76d23f264d2abeeb4/database/service_db.go#L127

krhubert
krhubert previously approved these changes Dec 3, 2018
@NicolasMahe
Copy link
Member Author

NicolasMahe commented Dec 3, 2018

@ilgooz > It's a bit better to do atomic reads (with transactions) for Get & All as well.
Transaction are only useful to group multiple reads and writes into a single atomic transaction.
If it only writes, batch is enough.
If it only one read, no need for transaction.
If it multiple reads, transactions are useful.
For the All function, i don't think we need transaction because Iterator is returning a snapshot of the database. So concurrent modifications are not affecting the iterator.

Here there is multiple reads

https://github.com/mesg-foundation/core/blob/64407a543a353286229e8ba76d23f264d2abeeb4/database/service_db.go#L127

Nice catch. Done in c486810aee9d9f325e136a3f98625c5d50615753

database/service_db_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

@krhubert krhubert merged commit 82b27c9 into dev Dec 3, 2018
@ilgooz ilgooz deleted the feature/service-alias branch December 3, 2018 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants