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

Fix broken ETS references #22

Merged
merged 5 commits into from
Jan 18, 2020
Merged

Fix broken ETS references #22

merged 5 commits into from
Jan 18, 2020

Conversation

Strech
Copy link
Owner

@Strech Strech commented Jan 17, 2020

This PR include 3 major changes:

  1. Avrora.Name module was moved to Avrora.Schema.Name
  2. Avrora.TestCase helper module gets cleanup_storage!/1 function to clean storage between tests
  3. Avrora.ETS module now supervisioned together with Avrora.Storage.Memory and owns all ETS tables generated by Avrora.Schema

Resolves: #21

* Implement flush callback for Avrora.Storage.Memory
It will give an oportunity to clean up memory storage
every time you run ex-unit
* Add Avrora.ETS to the supervision tree with "one for all"
  strategy close to Avrora.Storage.Memory
* Add new config setting Avrora.Config.ets_lib which holds
  module with `new/0` function
* Add Avrora.TestCase to clean Avrora.Storage.Memory state
  between tests
@Strech Strech merged commit 1b8017a into master Jan 18, 2020
@Strech Strech deleted the fix-avro-schema-store-binding branch January 18, 2020 18:58
@fxn
Copy link
Contributor

fxn commented Jan 21, 2020

I have read some source code.

Now I see that ETS is a backend for avro_schema_store.

Only feedback is that, in a first read, it seems slightly odd to me to use the generic API for ETS instead of :avro_schema_store.new/1, which would be consistent with the rest of API usage. The name of the module could also be more concise perhaps. You know what I mean, that all related to wrapping avro_schema_store feels coherent.

Also, that ETS table could have an informative name (can be passed to :avro_schema_store.new/1 as well), and then schemas would not need to carry references to the lookup table, because a name in the API calls would suffice.

Only some feedback since I said I would read the source code :). Of course, the current code does the job and my comments are details based mostly on personal preferences, no need to change anything unless you like some of those ideas.

Thanks for this library!

@Strech
Copy link
Owner Author

Strech commented Jan 21, 2020

@fxn Thanks for your effort, let me follow up:

  1. Only feedback is that, in a first read, it seems slightly odd to me to use the generic API for ETS instead of :avro_schema_store.new/1

Yes, I have that in mind, and first draft was doing exactly as you said, but :avro_schema_store.new/0

  1. The name of the module could also be more concise perhaps. You know what I mean, that all related to wrapping avro_schema_store feels coherent.

Again – you are right, I spend so much time to come up with a nice name and as you see I change it to ETS and that was a reason why I have changed from :avro_schema_store.new/0, to :ets.new/2 ... and I can find out a better name to kind-a wrap :avro_schema_store, I think I would switch back

  1. Also, that ETS table could have an informative name (can be passed to :avro_schema_store.new/1 as well), and then schemas would not need to carry references to the lookup table, because a name in the API calls would suffice.

Ok, here we have a more complicated situation. Since the same schema can be represented with few Avrora.Schema naming becomes cumbersome and to avoid collisions I decide that reference is a 100% working case. Also, naming will not save us from tables to be wiped, and the host process is still needed. So I consider names instead of references a costly change which will not give us many benefits.

P.S Just in case if you have in mind any name example for Avrora.ETS it will be great, but I think renaming and :ets to :avro_schema_store change is definitely a thing to do.

@fxn
Copy link
Contributor

fxn commented Jan 21, 2020

Ok, here we have a more complicated situation. Since the same schema can be represented with few Avrora.Schema naming becomes cumbersome and to avoid collisions I decide that reference is a 100% working case. Also, naming will not save us from tables to be wiped, and the host process is still needed. So I consider names instead of references a costly change which will not give us many benefits.

Can you elaborate on this? While reading the source code of avro_schema_store.erl I got the impression that the intended usage of the API was to have one ETS table in which all types are stored.

@Strech
Copy link
Owner Author

Strech commented Jan 22, 2020

@fxn Yep, they use schema store as one storage for all schemas you might have. But in our case, we can have a schema registry and the same schema can be represented with different versions. You can't have in one storage the same schema with different definitions.

That's why one schema is a small universe with its own storage where it will be decomposed on sub-definitions and lookup functionality becomes available from :avro_schema_store.


UPD: Schema decomposition happened when you add type to the :avro_schema_store, all records will be extracted and placed in a top-level to allow you quickly find them with :avro_schema_store.lookup_type/2

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.

Strange {:error, %ArgumentError{message: "argument error"}}
2 participants