Skip to content

Move TableId and NamespaceId to public API#950

Merged
ctubbsii merged 1 commit intoapache:masterfrom
ctubbsii:public-IDs
Feb 11, 2019
Merged

Move TableId and NamespaceId to public API#950
ctubbsii merged 1 commit intoapache:masterfrom
ctubbsii:public-IDs

Conversation

@ctubbsii
Copy link
Copy Markdown
Member

@ctubbsii ctubbsii commented Feb 9, 2019

  • Move TableId and NamespaceId to public API package for use in SPI and
    elsewhere, as needed.
  • Retain AbstractId (in public API) for common implementation, and
    because our internal namespace/table locking mechanism and unique ID
    generation is agnostic to the specific ID type.
  • Consolidate some constants for built-in tables/namespaces.
  • Shorten/simplify/reduce API method names.

* Move TableId and NamespaceId to public API package for use in SPI and
elsewhere, as needed.
* Retain AbstractId (in public API) for common implementation, and
because our internal namespace/table locking mechanism and unique ID
generation is agnostic to the specific ID type.
* Consolidate some constants for built-in tables/namespaces.
* Shorten/simplify/reduce API method names.
@ctubbsii ctubbsii added the v2.0.0 label Feb 9, 2019
@ctubbsii ctubbsii self-assigned this Feb 9, 2019
@ctubbsii ctubbsii requested a review from keith-turner February 9, 2019 18:01
@ctubbsii
Copy link
Copy Markdown
Member Author

ctubbsii commented Feb 9, 2019

This supersedes #938

}

public Table.ID getTableId() {
public TableId getTableId() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In 1.9 this returned a string.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This PR didn't make that change. It just made the change from an internal type to a public API type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah I know it didn't make the change... its just something I noticed. The change was made in the switch to Table IDs

Comment thread core/src/main/java/org/apache/accumulo/core/data/AbstractId.java
@mikewalch
Copy link
Copy Markdown
Member

It looks like the build is failing due to error below:

[ERROR] java/io/File.<init>(Ljava/io/File;Ljava/lang/String;)V reads a file whose location might be specified by user input [org.apache.accumulo.test.DumpConfigIT, org.apache.accumulo.test.DumpConfigIT] At DumpConfigIT.java:[line 64]At DumpConfigIT.java:[line 64] PATH_TRAVERSAL_IN

Copy link
Copy Markdown
Member

@mikewalch mikewalch left a comment

Choose a reason for hiding this comment

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

Just need to fix build

@ctubbsii
Copy link
Copy Markdown
Member Author

It looks like the build is failing due to error below:

[ERROR] java/io/File.<init>(Ljava/io/File;Ljava/lang/String;)V reads a file whose location might be specified by user input [org.apache.accumulo.test.DumpConfigIT, org.apache.accumulo.test.DumpConfigIT] At DumpConfigIT.java:[line 64]At DumpConfigIT.java:[line 64] PATH_TRAVERSAL_IN

Hmm, I'll take a look. I didn't see that fail, but I could have missed it. I thought the Travis build was failing due to sec-bugs timing out... which it frequently does in Travis.

@Override
public Text getTableId() {
return new Text(ke.getTableId().getUtf8());
return new Text(ke.getTableId().canonical());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be nice if we deprecated this method for a new one that returns the new TableId type. I think its confusing to have two different types returned across our API.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The more we can deprecate Text from our API, the better.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I opened up a follow on issue. #953

@ctubbsii
Copy link
Copy Markdown
Member Author

Will fix sec-bugs errors next. Merging now to facilitate follow-on issues which might otherwise cause conflicts later.

@ctubbsii ctubbsii merged commit 757f62b into apache:master Feb 11, 2019
@ctubbsii ctubbsii deleted the public-IDs branch February 11, 2019 22:02
@ctubbsii ctubbsii added this to the 2.0.0 milestone Jul 12, 2024
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