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

Use NewTableConfiguration (and also trivial code cleanup) #960

Merged
merged 2 commits into from Nov 10, 2017

Conversation

Projects
None yet
3 participants
@ctubbsii
Member

ctubbsii commented Oct 25, 2017

This PR consists of two commits (best left as two separate commits). The first cleans up some warnings, and formatting. The second makes use of NewTableConfiguration to create tables already pre-loaded with their initial properties at the time of creation.

After switching to NewTableConfiguration, I was reliably able to reproduce the bug which was fixed in #959 and also subsequently able to confirm it was fixed after that was merged in. So, I think this using this feature has merit.

This does require updating the minimum version of Accumulo to 1.7, which I think is fine, since 1.6 is EOL anyway. I also updated the Travis-CI config in the second commit so that tests are run in Travis with 1.8.1 routinely, since that's the current version of Accumulo.

@ctubbsii ctubbsii added this to the 1.2.0 milestone Oct 25, 2017

@ctubbsii ctubbsii self-assigned this Oct 25, 2017

@ctubbsii ctubbsii requested review from mikewalch and keith-turner Oct 25, 2017

IteratorSetting gcIter = new IteratorSetting(10, "gc", GarbageCollectionIterator.class);
GarbageCollectionIterator.setZookeepers(gcIter, config.getAppZookeepers());
private String encodeColumnFamily(Bytes cf) {

This comment has been minimized.

@keith-turner

keith-turner Oct 26, 2017

Contributor

I think this code was copied from Accumulo? We should open issues in Accumulo to improve the ability to set iterators and locality groups on new table config. I'll open those issues.

This comment has been minimized.

@ctubbsii

ctubbsii Oct 26, 2017

Member

Modified a bit, but yeah, mostly copied. Agreed on better API in Accumulo for this.

This comment has been minimized.

EnumSet.of(IteratorUtil.IteratorScope.majc, IteratorUtil.IteratorScope.minc));
private Map<String, String> initializeApplicationTableProps() {
Map<String, String> ntcProps = new HashMap<>();
ntcProps.put("table.group.notify", encodeColumnFamily(ColumnConstants.NOTIFY_CF));

This comment has been minimized.

@keith-turner

keith-turner Oct 26, 2017

Contributor

It would be nice to create constants for these props in AccumuloProps class.

@keith-turner

This comment has been minimized.

Contributor

keith-turner commented Oct 27, 2017

I was thinking of manually testing this changes, but then it occurred to me it would be nice to test the following in a IT

  • That locality groups are setup after init. Test this by requesting loc groups using Accumulo public API.
  • That iterators were properly setup after init. Test this by getting tablets iters using public API.

These test could possibly be done in FluoAdminImplIT.

The reason I wanted to test manually is because I had angst about the copied code from Accumulo. If iterators were not properly set, then other ITs would probably catch that. However if loc groups were not properly set, nothing would catch that (it would just cause performance problems). So I think checking loc gorups after init is a good test even w/o these changes. These test could be done in this issue or a follow on issue.

@keith-turner

This comment has been minimized.

Contributor

keith-turner commented Oct 31, 2017

@ctubbsii were you planning on making any other changes to this issue?

@ctubbsii

This comment has been minimized.

Member

ctubbsii commented Oct 31, 2017

@keith-turner Maybe. I was going to take a look at the review comments when I got a chance. I have been distracted by other priorities.

@ctubbsii

This comment has been minimized.

Member

ctubbsii commented Nov 1, 2017

Warnings are bothering me, so I'm going to fix those first, then I'll add the test for the locality group being set correctly and review the suggestion for adding constants for some of the string literals.

@ctubbsii

This comment has been minimized.

Member

ctubbsii commented Nov 9, 2017

Added commit to cover feedback from review. Will rebase and squash some of these together, upon review approval, just to clean up a bit before merging, but for now, I just added the one commit, for easier review.

@@ -21,5 +21,8 @@
public static final String TABLE_CLASSPATH = "table.classpath.context";
public static final String TABLE_BLOCKCACHE_ENABLED = "table.cache.block.enable";
public static final String TABLE_FORMATTER_CLASS = "table.formatter";
public static final String TABLE_GROUP_PREFIX = "table.group.";

This comment has been minimized.

@keith-turner

keith-turner Nov 10, 2017

Contributor

Thanks. This is much nicer.

Map<String, Set<Text>> localityGroups =
conn.tableOperations().getLocalityGroups(config.getAccumuloTable());
Assert.assertEquals("Unexpected locality group count.", 1, localityGroups.size());
Entry<String, Set<Text>> localityGroup = localityGroups.entrySet().iterator().next();

This comment has been minimized.

@keith-turner

keith-turner Nov 10, 2017

Contributor

I think using guava's Iterables.getOnlyElement could shorten this test a bit. Since that guava method throws an exception if the Iterable does not have only one element, then there is no need to check for that case.

This comment has been minimized.

@ctubbsii

ctubbsii Nov 10, 2017

Member

I had forgotten about Iterables.getOnlyElement, but had I remembered, I think I probably would have still written it the way I did for a few reasons:

Since Java 8, I've tried to make a deliberate effort to avoid Guava functional stuff in favor of Java 8 equivalents, since all of Guava is pre-lambdas, and some of it doesn't play nice with Java 8 functional stuff. It's also not clear to me how much Guava is going to change to deal with Java 8 over time, so I don't know how much it makes sense to double down on Guava usage when it isn't essential for the desired functionality. I don't want to lock in to something which will break in future Guava versions. Iterables, notably, doesn't play nice with (isn't easily interchangeable with) Streams, so I've intentionally avoided anything with Iterables. So, I don't want to become too reliant on Guava in new code.

I'm also hesitant to use Guava for small things when those small things on their own wouldn't warrant the extra dependency. In this case, it's probably already on the test class path, so that doesn't matter that much.

Anyway, I don't know if any of that's a good argument against using Iterables.getOnlyElement or not. It's just why I might not have used it had I remembered it existed. Maybe you can let me know what you think. I can certainly change it.

This comment has been minimized.

@keith-turner

keith-turner Nov 10, 2017

Contributor

I am ok with using it or not using it.

ColumnConstants.NOTIFY_LOCALITY_GROUP_NAME, localityGroup.getKey());
Assert.assertEquals("'notify' locality group does not contain exactly 1 column family.", 1,
localityGroup.getValue().size());
Text colFam = localityGroup.getValue().iterator().next();

This comment has been minimized.

@keith-turner

keith-turner Nov 10, 2017

Contributor

Could also use Iterables.getOnlyElement here.

ctubbsii added some commits Nov 10, 2017

Minor code cleanup and remove compiler warnings
Remove all compiler warnings from javac on the command-line and in
Eclipse IDE

* Remove unused imports
* Remove redundant generic type specifications
* Add missing Override annotations
* Remove redundant semi-colon
* Apply formatter changes
* Fix broken javadoc
* Work around deprecation warnings by creating temp variables and
  suppressing
* Work around https://bugs.openjdk.java.net/browse/JDK-8032211 by using
  fully qualified class names instead of imports when a deprecated class
  must be used
* Add missing hashCode implementations when equals is overridden, so
  there aren't suprising behaviors (throw UnsuppotedOperationException
  instead)
* Remove dead/unused private code
Use NewTableConfiguration when creating tables
Use Accumulo's NewTableConfiguration when creating new application
tables. This way, the initial table configuration, including locality
group settings and compaction iterators, are already set on the table at
the moment it is created, eliminating the possibility of race conditions
related to setting configuration after the table is created.

This change requires at least Accumulo 1.7.0.

Travis-CI configuration was updated to test against the latest version
of Accumulo (1.8.1) with the corresponding Thrift version (0.9.3)

Include test to verify locality group serialization can be deserialized
by Accumulo's public API, and adds some constants to refer to Accumulo
properties and column family and locality group names.
@ctubbsii

This comment has been minimized.

Member

ctubbsii commented Nov 10, 2017

Rebase'd and squashed commits, in prep for merge.

@asfgit asfgit merged commit eb0889b into apache:master Nov 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ctubbsii ctubbsii deleted the ctubbsii:use-ntc branch Nov 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment