Return-Path: X-Original-To: apmail-hive-dev-archive@www.apache.org Delivered-To: apmail-hive-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 5A1DB106BB for ; Wed, 9 Apr 2014 01:53:35 +0000 (UTC) Received: (qmail 49752 invoked by uid 500); 9 Apr 2014 01:53:34 -0000 Delivered-To: apmail-hive-dev-archive@hive.apache.org Received: (qmail 49644 invoked by uid 500); 9 Apr 2014 01:53:33 -0000 Mailing-List: contact dev-help@hive.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@hive.apache.org Delivered-To: mailing list dev@hive.apache.org Received: (qmail 49634 invoked by uid 99); 9 Apr 2014 01:53:33 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 09 Apr 2014 01:53:33 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id CDD9B1D5E21; Wed, 9 Apr 2014 01:53:28 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============8565554213897678476==" MIME-Version: 1.0 Subject: Re: Review Request 19754: Defines a api for streaming data into Hive using ACID support. From: "Lefty Leverenz" To: "Roshan Naik" , "Lefty Leverenz" , "hive" Date: Wed, 09 Apr 2014 01:53:28 -0000 Message-ID: <20140409015328.12752.30217@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Lefty Leverenz" X-ReviewGroup: hive X-ReviewRequest-URL: https://reviews.apache.org/r/19754/ X-Sender: "Lefty Leverenz" References: <20140408182808.13537.52021@reviews.apache.org> In-Reply-To: <20140408182808.13537.52021@reviews.apache.org> Reply-To: "Lefty Leverenz" X-ReviewRequest-Repository: hive-git --===============8565554213897678476== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19754/#review39817 ----------------------------------------------------------- hcatalog/streaming/pom.xml typo: artifectId should be artifactId hcatalog/streaming/pom.xml typo: artifectId should be artifactId hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/AbstractRecordWriter.java suggestion for Txnid: either spell out transaction ("transaction ID" -- preferable) or use capital I like the parameter (TxnId) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/AbstractRecordWriter.java Why does the parameter name have both-caps "ID" for maxTxnID while it's init-cap "Id" for minTxnId? Are parameter names case-sensitive? Also a suggestion for Txnid in description: either spell out transaction ("transaction ID" -- preferable) or use capital ID like the parameter (TxnID). hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/AbstractRecordWriter.java Same question as line 108 about minTxnId vs maxTxnID capitalization hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java Nit: period at the end (next line too) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java Editorial nits: Please capitalize "nulls" and end the second sentence with a period (next line) just for consistency with the first sentence. hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java Grammar nit: Remove "s" from "indicates" because the subjects are plural. hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java Consistency nit: Since other param descriptions are capitalized on the first word, please do the same here. Bonus points if you capitalize all the param descriptions in this patch, but I'm not going to comment on all of them. You could argue for a rule that only capitalizes full sentences and proper nouns like Hive, in which case [pun alert] it's okay to leave "input" uncapitalized. But I favor visual consistency over rule consistency, except when I'm inconsistent. Terminal periods aren't essential (given the typical style of javadocs) but they're recommended when a description has multiple sentences. Hm, but that's inconsistent with my visual consistency preference. Why am I wasting your time with this trivia? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java should "endpoint" be explained? (your call) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java Editorial nit: "non existing" seems okay in this context, but "nonexistent" is the real word (your choice). Consistency nit again: Since other exception descriptions are capitalized on the first word, please do the same here. hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java ditto line 57 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java ditto line 58 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java ditto line 59 (capitalization) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java ditto line 60 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java Hive nit: please capitalize "hive" Editorial nits: please capitalize "a" and perhaps spell out configuration in "conf object" unless conf is the proper term for the object hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java ditto line 65 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java ditto line 59 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java ditto line 60 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java ditto line 79 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java ditto line 59 (capitalization) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java ditto line 65 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java do you want to document the return? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java do you want to document tbl? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java add @param conf hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java Hive nit: please capitalize hive (also, end point is a single word elsewhere) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java missing a method description hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java why is the comma here rather than on the last line? (just curious) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java want to document the return? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java proxyUser isn't a parameter of this method (does that matter?) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java IOException isn't in the throws list hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java want to document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java exception InvalidTable isn't documented hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java Nits: first comma belongs on previous line, second comma doesn't need space before it hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java Hive nit: please capitalize hive hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java want to document the return? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java proxyUser isn't a parameter of this method (does that matter?) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java IOException isn't in the throws list hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java want to document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java exception InvalidTable isn't documented hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java Nits: first comma belongs on previous line, second comma doesn't need space before it hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java please change hdfs and hive to HDFS and Hive hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java add a period after "null" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document the return? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java IOException isn't in the throws list hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java @param conf & @throws InvalidTable aren't documented hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java unnecessary space before a comma hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java no method description ... but this is private so doc not crucial hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java Nit: end point is one word elsewhere hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java Description starts "of prody user" -- should be "UGI of proxy user" (and could add description that starts on line 134). hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java comma belongs on previous line hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java want to document the return? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java throws list has StreamingException, not StreamingIOFailure hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java proxyUser isn't a parameter of this method (does that matter?) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java comma belongs on previous line hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java no param descriptions, but this is a private method so javadoc not published hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java comma belongs on previous line hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java @throws ImpersonationFailed & InterruptedException not documented hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document the return? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java capitalize "get" & change "tramsaction" to "transaction" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document the return? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java insert "in" --> "remaining [in] this batch" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java throws list has StreamingException, not StreamingIOFailure hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java SerializationError is not in the throws list hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java throws list has StreamingException, not StreamingIOFailure hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java 3 exceptions not documented hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java comma belongs on previous line hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java throws list has StreamingException, not StreamingIOFailure hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java 2 exceptions not documented hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/RecordWriter.java Hive nit: capitalize "hive" (also change "Writes" to "Write" for consistency -- I didn't comment on a few other instances of this because these nits are overwhelming and mostly unimportant) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/RecordWriter.java optional: spell out transaction for Txn hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/RecordWriter.java @throws StreamingException not documented hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/RecordWriter.java ditto line 28 (exception doc) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/RecordWriter.java ditto line 28 (exception doc) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/RecordWriter.java ditto line 28 (exception doc) and two params not documented (also, different capitalization of id in minTxnId & maxTxnID) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/RecordWriter.java ditto line 28 (exception doc) hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StreamingConnection.java omit "is" at beginning of definition, then capitalize "a" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StreamingConnection.java document the return? -- yes, done in line 38 so this line isn't needed hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StreamingConnection.java document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StreamingConnection.java InvalidPartition not in the throws list, but InterruptedException needs to be documented hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StreamingConnection.java document this exception? hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StrictJsonWriter.java no method description and no exception descriptions hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StrictJsonWriter.java no method description and no exception descriptions hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StrictJsonWriter.java Nit: end point is a single word elsewhere hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StrictJsonWriter.java conf could be spelled out (configuration) in the description hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StrictJsonWriter.java skimpy definition but okay for private method hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StrictJsonWriter.java ditto line 88 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StrictJsonWriter.java ditto hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StrictJsonWriter.java ditto line 88 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StrictJsonWriter.java ditto line 88 hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java spell out Txn hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java "in interrupted" (should be "is") hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java Nit: capitalize "get" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java Nit: capitalize "get" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java "in" --> "is" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java "in" --> "is" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java "remaining [in] this batch" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java "in" --> "is" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java "in" --> "is" hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java capitalize "hive" and spell out txn hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java "in" --> "is" - Lefty Leverenz On April 8, 2014, 6:27 p.m., Roshan Naik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19754/ > ----------------------------------------------------------- > > (Updated April 8, 2014, 6:27 p.m.) > > > Review request for hive. > > > Bugs: HIVE-5687 > https://issues.apache.org/jira/browse/HIVE-5687 > > > Repository: hive-git > > > Description > ------- > > Defines an API for streaming data into Hive using ACID support. > > > Diffs > ----- > > hcatalog/pom.xml 50ce296 > hcatalog/streaming/pom.xml PRE-CREATION > hcatalog/streaming/src/docs/package.html PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/AbstractRecordWriter.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/ConnectionError.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/DelimitedInputWriter.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HeartBeatFailure.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/HiveEndPoint.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/ImpersonationFailed.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/InvalidColumn.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/InvalidPartition.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/InvalidTable.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/InvalidTrasactionState.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/PartitionCreationFailed.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/QueryFailedException.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/RecordWriter.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/SerializationError.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StreamingConnection.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StreamingException.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StreamingIOFailure.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/StrictJsonWriter.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatch.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionBatchUnAvailable.java PRE-CREATION > hcatalog/streaming/src/java/org/apache/hive/hcatalog/streaming/TransactionError.java PRE-CREATION > hcatalog/streaming/src/test/org/apache/hive/hcatalog/streaming/StreamingIntegrationTester.java PRE-CREATION > hcatalog/streaming/src/test/org/apache/hive/hcatalog/streaming/TestDelimitedInputWriter.java PRE-CREATION > hcatalog/streaming/src/test/org/apache/hive/hcatalog/streaming/TestStreaming.java PRE-CREATION > hcatalog/streaming/src/test/sit PRE-CREATION > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java 1bbe02e > packaging/pom.xml de9b002 > packaging/src/main/assembly/src.xml bdaa47b > > Diff: https://reviews.apache.org/r/19754/diff/ > > > Testing > ------- > > Unit tests included. Also done manual testing by streaming data using flume. > > > Thanks, > > Roshan Naik > > --===============8565554213897678476==--