Clarify Accumulo API by keith-turner · Pull Request #28 · apache/accumulo

keith-turner · 2015-04-14T23:45:56Z

This pull request attempts to clarify what the Accumulo API is. The existing API statement does not include all types that a user would realistically need to use. A new API statement is proposed in 2f7d095.

This pull request also prunes (via deprecation) things from the API that were not intended to be in the API, but ended up there. For example parts of the API that referenced non API types and as a result may not be stable were deprecated.

I am requesting this review on GH instead of ReviewBoard because I thought it would be much easier to review the commits in this PR individually rather than squasing them for RB. For example the commit that deprecated and moved KeyExtent was very noisy and would make reviewing other changes difficult.

One thing that I was not able to deprecate move was o.a.a.c.security.crypto. I would have liked to have moved this to o.a.a.c.crypto. Its not really client API code, its more server side extension code. However the class names are possibly referenced by user config. Also users may have extended the classes. I added an exception for the package in the README's API stmt. Does anyone have any ideas about making this less confusing w/o causing existing users any pain.

asfbot · 2015-04-14T23:59:45Z

Accumulo-Pull-Requests #39 FAILURE
Looks like there's a problem with this pull request

keith-turner · 2015-04-15T14:27:21Z

@ctubbsii I meant to do something about secrutiy.Credentials in this change, but forgot. I was thinking of moving it to another package so that it would not be in public API. I don't think its using any public API methods. Need to do something about it because it will end up in public API.

asfbot · 2015-04-15T17:00:12Z

Accumulo-Pull-Requests #40 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-04-15T17:27:33Z

Accumulo-Pull-Requests #41 SUCCESS
This pull request looks good

ctubbsii · 2015-04-15T20:28:02Z

@keith-turner Moving it to another package makes sense to me. It's not intended to be public API.

asfbot · 2015-04-15T21:59:59Z

Accumulo-Pull-Requests #42 SUCCESS
This pull request looks good

ericnewton · 2015-04-16T17:25:18Z

core/src/main/java/org/apache/accumulo/core/data/TabletID.java

I can't say that I love the name "TabletID". Maybe TableRowRange?

Interface and methods need javadoc.

Not a fan of TabletID either, but it does make sense (it uniquely identifies a Tablet). TabletIdentifier? I dunno.

I don't really like TabletID or TableRowRange that much. Personally I would prefer TabletID because its shorter.

I would like to hear more suggestions.

ericnewton · 2015-04-16T17:38:33Z

Pull looks good. Thanks for putting in the tedious work to clarify the API.

joshelser · 2015-04-17T03:19:08Z

README.md

Can this be clarified to include package-protected as that's what we've been going on to date?

I didn't think package-protected had been included to date. has it?

I thought you were the one who argued that package-protected still allows users to access the class/methods/whatever. Maybe I'm misremembering things? Anything not private == fair-game is the assumption I've been operating under at least.

If I did then past-me was wrong. :)

package-protected stuff (meaning java members with no access modifier, just to make sure we're on the same page) is only accessible if downstream folks make classes in our package space. IMHO that's a party foul and folks should expect such activity to be unsupported by us.

Haha, yeah, I remember having that same reaction of "really?", but who knows. It wouldn't be the first weird thing I dreamed up. Since you guys took the time to clarify this, let's try to make this as bullet-proof as possible. Sounds like package-protected should be mentioned with private as being inaccessible.

The only potential drawback I can think of is making the stmt so long that no one reads its

I think this is one time where it benefits us to be as specific as possible (see how often we had these "oops" moments already, and that Sean and I didn't know which was correct).

I have heard @ctubbsii mention that we build sealed jars that would prevent users from doing this.

Sealed jars are only relevant if the user downloads binary artifacts from us, not from users building the code themselves, no?

Sealed jars are only relevant if the user downloads binary artifacts from us

I have no idea. If someone is using Accumulo's maven pom to build, I am not sure under what circumstances it seals. Someone could create jars w/o using the pom I suppose.

I didn't think we sealed the jars by default but instead during the apache-release profile. I could be mistaken though.

Fixed in d7ffec1

Thanks, Keith. Also, sorry all for confusion in using the wrong terminology. For some reason I kept thinking package-private was called package-protected.

joshelser · 2015-04-17T03:45:14Z

Overall this looks pretty good, but needs a little bit of touch up still. Nice work Keith and Christopher, this took some doing. Some general thoughts:

DelegationToken move is awkward to me. I think this should be in public API and we just hide the impl details.
Regarding what to do with the crypto package, can we get @ohshazbot attention maybe? Perhaps he can help give some insight. I think it was designed to be extendable by users (CryptoModule and SecretKeyEncryptionStrategy), but there seems to be a lot of other stuff in there that shouldn't be (or be in an impl). Maybe we can get this figured out for 1.7.0 so we can start the deprecation cycle to remove the bits which shouldn't be there?

madrob · 2015-04-17T14:46:52Z

Regarding what to do with the crypto package, can we get @ohshazbot attention maybe? Perhaps he can help give some insight. I think it was designed to be extendable by users (CryptoModule and SecretKeyEncryptionStrategy), but there seems to be a lot of other stuff in there that shouldn't be (or be in an impl). Maybe we can get this figured out for 1.7.0 so we can start the deprecation cycle to remove the bits which shouldn't be there?

I know of somebody working with the crypto stuff, I'll reach out and make sure this gets noticed.

asfbot · 2015-04-17T15:43:54Z

Accumulo-Pull-Requests #44 FAILURE
Looks like there's a problem with this pull request

joshelser · 2015-04-17T15:45:22Z

core/src/main/java/org/apache/accumulo/core/client/impl/DelegationTokenImpl.java

Fixed in 2f5e415. How does that look? Javadoc look good?

The fix looks great. I don't think we need since or quite as verbose a javadoc here since it's already contained (and better worded) in the DelegationToken interface

fixed in 9d5a6f5

asfbot · 2015-04-17T15:51:29Z

Accumulo-Pull-Requests #45 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-04-17T15:56:47Z

Accumulo-Pull-Requests #46 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-04-17T16:41:14Z

Accumulo-Pull-Requests #50 SUCCESS
This pull request looks good

ctubbsii · 2015-04-17T17:16:23Z

@joshelser Regarding what to do about the crypto package... that stuff is still marked as experimental, and some of it is still incomplete for full on-disk encryption (ACCUMULO-981), so whatever we do, I think for now, it should not be moved into the public API, because it may require unstable API changes to finish that work.

ctubbsii · 2015-04-17T17:16:52Z

If we do move it to the public API, it would need to be accompanied with a Beta annotation or something similar.

joshelser · 2015-04-17T17:26:37Z

that stuff is still marked as experimental

That is a very good point. Putting something marked as experimental into public API would be precedent setting. I would agree with you that it should not be added until it is actually capable of performing complete encryption of Accumulo data.

keith-turner · 2015-04-17T18:22:33Z

that stuff is still marked as experimental,

That is a good point. I will wait and see if there are any other comments on this. Moving it and removing the exception from the README would be nice.

asfbot · 2015-04-17T18:30:45Z

Accumulo-Pull-Requests #53 SUCCESS
This pull request looks good

joshelser · 2015-04-17T21:40:49Z

Said it in irc, but, for the record, I'm +1 with these changes + no crypto stuff in public API.

ctubbsii · 2015-04-17T21:47:38Z

I'm +1 with these changes

Also +1

no crypto stuff in public API.

Same, at least for now, until it's 100% feature complete and no longer experimental, because until that time, we can't really make strong API guarantees about it. I also don't see the sense in moving it, because it'd unnecessarily disrupt those taking advantage of it today as an experimental feature.

Use plugin to check for API problems

The following changes were made. * Fix API problems with IsolatedScanner * Deprecated public inner class in ClientSiderIteratorScanner that was not intended to be in public API * ACCUMULO-3488 Deprecated KeyExtent from public API * Deprecated use of Property in ClientConfiguration * deprecated ZooKeeperInstance.lookupIntanceName(ZooCache, UUID) * deprecated IteratorUtil.getProperty(IteratorScope) * deprecated class ComparableBytes * ACCUMULO-3724 moved DelegationToken and AuthenticationTokenIdentifier out of public API * deprecated getTabletLocator() in both AbstractInputFormat classes * deprecated getAccumuloConfiguration() in both AccumuloFileOutputFormat classes * deperecated and moved VisibilityConstraint * updated README to accurately communicate Accumulo's API. Update APILyzer config to enforce ACCUMULO-3720 * moved Credentials from core.security to core.client.impl

keith-turner · 2015-04-17T22:52:15Z

I rebased on master and squished most of the commits. After rebasing on master, APILyzer found some new problems.

  FIELD                org.apache.accumulo.core.client.mapred.AbstractInputFormat$AbstractRecordReader split                               org.apache.accumulo.core.client.mapreduce.impl.AccumuloInputSplit
  METHOD_PARAM         org.apache.accumulo.core.client.mapred.RangeInputSplit       updateSplit(...)                    org.apache.accumulo.core.client.mapreduce.impl.AccumuloInputSplit
  FIELD                org.apache.accumulo.core.client.mapreduce.AbstractInputFormat$AbstractRecordReader split                               org.apache.accumulo.core.client.mapreduce.impl.AccumuloInputSplit
  METHOD_PARAM         org.apache.accumulo.core.client.mapreduce.RangeInputSplit    updateSplit(...)                    org.apache.accumulo.core.client.mapreduce.impl.AccumuloInputSplit

asfbot · 2015-04-17T23:01:13Z

Accumulo-Pull-Requests #55 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-04-21T22:28:19Z

Accumulo-Pull-Requests #62 SUCCESS
This pull request looks good

joshelser · 2015-04-21T22:34:46Z

Newest changes look ok too. A few are a little confusing (notably (re)introducing protected RangeInputSplitsplit), but I assume that they were potentially visible before.

Also, do we want all the members on AccumuloInputSplit to be private and not package-private?

keith-turner · 2015-04-21T22:43:05Z

Also, do we want all the members on AccumuloInputSplit to be private and not package-private?

Well I think some of the public members of AccumuloInputSplit used to be public methods if RangeInputSplit. I think some public members of AccumuloInputSplit were not intened to be in public API, but end up there because RangeInputSplit extends AccumuloInputSplit. I think public API types extending impl types is nothing but trouble.

I think another issue needs to be opened. I think the map reduce public API is a mess. Too much of the implementation is in the API, makes it very hard to add features.

keith-turner · 2015-04-27T15:12:37Z

These changes were merged

ericnewton reviewed Apr 16, 2015
View reviewed changes

joshelser reviewed Apr 17, 2015
View reviewed changes

ACCUMULO-3720 Use apilyzer-maven-plugin

3adf1dd

Use plugin to check for API problems

keith-turner force-pushed the api-cleanup branch from 569dee5 to 0002c6f Compare April 17, 2015 22:49

ACCUMULO-3602 ACCUMULO-3657 Minimize AccumuloInputSplit in API

8c046ba

keith-turner closed this Apr 27, 2015

keith-turner deleted the api-cleanup branch May 24, 2016 21:58

karthick-rn mentioned this pull request Apr 1, 2020

Accumulo master hangs after TLS on ZK #1578

Closed

keith-turner pushed a commit to keith-turner/accumulo that referenced this pull request Jun 23, 2022

Attempting to fix the MR API ConsistencyLevel issue (apache#28)

c7dd148

Conversation

keith-turner commented Apr 14, 2015

Uh oh!

asfbot commented Apr 14, 2015

Uh oh!

keith-turner commented Apr 15, 2015

Uh oh!

asfbot commented Apr 15, 2015

Uh oh!

asfbot commented Apr 15, 2015

Uh oh!

ctubbsii commented Apr 15, 2015

Uh oh!

asfbot commented Apr 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericnewton commented Apr 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshelser commented Apr 17, 2015

Uh oh!

madrob commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

ctubbsii commented Apr 17, 2015

Uh oh!

ctubbsii commented Apr 17, 2015

Uh oh!

joshelser commented Apr 17, 2015

Uh oh!

keith-turner commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

joshelser commented Apr 17, 2015

Uh oh!

ctubbsii commented Apr 17, 2015

Uh oh!

keith-turner commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

asfbot commented Apr 21, 2015