-
Notifications
You must be signed in to change notification settings - Fork 445
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
ACCUMULO-4813 New bulk import process and API #436
Conversation
* Provide initial API * Provide some initial client code
* New Bulk Import Fate REPO * GSON serialize & implement Bulk class * Deprecated old BulkImporter * Created BulkLoadIT for new technique * TODO add more tests to BulkLoadIT
* In load RPC aow sending multiple file per tablet instead of one * Now saving mapping file to HDFS on client side * Simplified JSON serialization * Refactored new and old bulk import code into two packages * Now only iterate over mapping in master, no longer load into memory
* Fix bug for single tablet ingest * Fix BulkImportMove * Created 2 new tests in BulkLoadIT * Catch ExecutionException in Move
* Load files from this directory | ||
* @return ImportSourceOptions | ||
*/ | ||
ImportSourceOptions from(String directory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should return ImportExecutorOptions
|
||
/** | ||
* Read Json array of Bulk.Mapping objects and return SortedMap of the bulk load mapping | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update this javadoc
OTHER(7), | ||
NAMESPACE_EXISTS(8), | ||
NAMESPACE_NOTFOUND(9), | ||
INVALID_NAME(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to put the new type at the END of the list so the existing numbers don't change.
TABLE_CANCEL_COMPACT(12), | ||
NAMESPACE_CREATE(13), | ||
NAMESPACE_DELETE(14), | ||
NAMESPACE_RENAME(15); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to put the new type at the END of the list so the existing numbers don't change.
* Normal operations, like bulk imports, will grab the read lock and prevent merges (writes) while | ||
* they run. Merge operations will lock out some operations while they run. | ||
* Normal operations, like bulkDir imports, will grab the read lock and prevent merges (writes) | ||
* while they run. Merge operations will lock out some operations while they run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like find/replace caught some unintentional javadoc. We didn't modify this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments so far. Might do a longer review if I get time later.
// TODO need to handle case of file existing | ||
BulkSerialize.writeLoadMapping(mappings, srcPath.toString(), p -> fs.create(p)); | ||
|
||
List<ByteBuffer> args = Arrays.asList(ByteBuffer.wrap(tableName.getBytes(UTF_8)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably send tableId instead of name.
} | ||
|
||
int nThreads; | ||
if (threads.toUpperCase().endsWith("C")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably move this configuration type parsing into the ConfigurationTypeHelper.
* Update ordering of new thrift objects * Move Property parsing to ConfigTypeHelper * Fix Javadoc
* Updated documentation * Fixed bug in API * Unit tested merge detection code (and found bugs in the proccess) * Updated existing bulk IT to use new and old APIs * Addressed many TODOs
Since this PR was merged, should the ACCUMULO-4813 JIRA issue be closed now? |
This is a big change to how bulk import works. This new API does the following for bulk import.
The new API and functionality is complete, however the following still needs to be done.
After this PR is finished with review, will open issues for these and anything else that comes up.