Skip to content

Move table#938

Closed
keith-turner wants to merge 3 commits intoapache:masterfrom
keith-turner:move-table-id
Closed

Move table#938
keith-turner wants to merge 3 commits intoapache:masterfrom
keith-turner:move-table-id

Conversation

@keith-turner
Copy link
Copy Markdown
Contributor

No description provided.

The SPI package and table ID are both new in 2.0.   I have been cleaning up
the use of internal types in plugabble interfaces and noticed alot of
pluggable interfaces use table ID as string.  Using a type instead of string
would be better.  So I think making this new 2.0 type available to pluggable
interfaces makes sense.  This commit moves table ID to an SPI package.
@milleruntime
Copy link
Copy Markdown
Contributor

Does it make more sense to move it to o.a.a.core.data package? Part of the reason we put it in a Table class was so we could eventually create a Table API within it.

@keith-turner
Copy link
Copy Markdown
Contributor Author

Does it make more sense to move it to o.a.a.core.data package?

What in the public API would use it? I don't think it makes sense to have it in the public API if nothing uses it.

@keith-turner
Copy link
Copy Markdown
Contributor Author

Does it make more sense to move it to o.a.a.core.data package?

I think there is a compelling reason to have the type in the SPI now because internal plugins are given table IDs. Currently in the public API tableID are never used for any operations and are only available for informational purposes. I think it it would only make sense to have a table ID type in the API if non-informational APIs needed it.

@mikewalch
Copy link
Copy Markdown
Member

There is a conflict that needs to be resolved.

@milleruntime
Copy link
Copy Markdown
Contributor

Does it make more sense to move it to o.a.a.core.data package?

What in the public API would use it? I don't think it makes sense to have it in the public API if nothing uses it.

Nothing right now. I was saying that if we are going to have a public Table class, wouldn't it make sense to have it with the other data classes like Key and Column. The SPI packages would be the only thing that uses it now. If you put Table in the SPI and if we ever come up with a Table API then we would have two Table classes one in SPI and one in with the data classes.

A class like ScanInfo in the SPI would depend on org.apache.accumulo.core.data.Table like it does now with org.apache.accumulo.core.data.Column

@keith-turner
Copy link
Copy Markdown
Contributor Author

I don't think its a good idea to put it in the API now with nothing in the API using it. If we do use it in the API in the future, we may want do things differently when we actually try to use it.

@keith-turner
Copy link
Copy Markdown
Contributor Author

keith-turner commented Feb 5, 2019

It may make sense to have something in the public API in the future, but that is not something I would want to take on for 2.0. I can see how possibly having an API and SPI table id type in the future could be problematic.

However we may never have the type in the API or the SPI and as a result SPI code may be buggier.

I'll hold off on this for now.

@ctubbsii
Copy link
Copy Markdown
Member

ctubbsii commented Feb 6, 2019

My intent for this ID type was always to have it in the public API (as an immutable strongly typed surrogate for the human-readable "string" ID), but was holding off until it could be included as part of first-class citizen Table and Namespace object representation of a table (or namespace). (example: create table could return a Table, and you could do things like table.renameTo(newName), table.scan(), and table.getId()), but I didn't think that the ID would be useful by itself, so when @milleruntime worked on it, I suggested he keep it internal only for now. But for SPI, it seems like there's a good reason to go ahead and make it public API.

I would be in favor of putting it in o.a.a.core.data as TableId and NamespaceId, getting rid of the abstract base class, and the removing it from its containing outer class (which can be removed).

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.

4 participants