git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Review Request 67023: HIVE-18117: Add a new Test Driver "TestErasureCodingHDFSCliDriver" that can be used to run tests over hdfs directories that employ Erasure Coding.



> On May 10, 2018, noon, Sahil Takiar wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java
> > Lines 674 (patched)
> > <https://reviews.apache.org/r/67023/diff/1/?file=2018287#file2018287line674>
> >
> >     is this necessary since you set the cluster type to mr above?
> 
> Andrew Sherman wrote:
>     Ha good question. Yes it is necessary as setClusterType() does not always set the cluster type :-( - it allows the cluster type to overridden with -Dclustermode=xxx

interesting, should we handle other cluster types like Spark or MR too?


> On May 10, 2018, noon, Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java
> > Lines 89 (patched)
> > <https://reviews.apache.org/r/67023/diff/1/?file=2018290#file2018290line89>
> >
> >     an `echo` command seems like a useful feature for all q-tests, does it need to be EC specific?
> 
> Andrew Sherman wrote:
>     I put this in as I want it. I did think about putting it somewhere else but there is no generic test-only CommandProcessor. Do you see an obvious place to put it?

if its not easy to add for others, then just leave it


> On May 10, 2018, noon, Sahil Takiar wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
> > Lines 545 (patched)
> > <https://reviews.apache.org/r/67023/diff/1/?file=2018292#file2018292line545>
> >
> >     whats the cache for? can q-tests even specify custom URIs? whats the use case for support multiple fs URIs?
> 
> Andrew Sherman wrote:
>     OK I admit I copied this code from the way hdfsEncryptionShims works without fully understanding it.

can we delete it then? i didn't realize this would require modifying code outside of itests, so I think we should make any changes to core Hive as minimal as possible


> On May 10, 2018, noon, Sahil Takiar wrote:
> > shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java
> > Lines 699 (patched)
> > <https://reviews.apache.org/r/67023/diff/1/?file=2018300#file2018300line699>
> >
> >     whats the use case for this dummy class? so we can run ec tests on hadoop versions that don't support ec? wouldn't be just disable the clidriver entirely for versions that don't support ec?
> 
> Andrew Sherman wrote:
>     I'm imagining wanting to run a test on bith EC and non-EC

i thought the `NoopHdfsErasureCodingShim` was used when "the hadoop version does not support hdfs Erasure Coding". u can still run on EC and non-EC folders without this, right?


> On May 10, 2018, noon, Sahil Takiar wrote:
> > shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java
> > Lines 741 (patched)
> > <https://reviews.apache.org/r/67023/diff/1/?file=2018300#file2018300line741>
> >
> >     since we have a cache anyway, would it make more sense to just remove this and make it a loading cache?
> 
> Andrew Sherman wrote:
>     I don't know, maybe. There is some advantage in keeping the same structure as the HdfsEncryptionShim.

see comment above about removing the cache


> On May 10, 2018, noon, Sahil Takiar wrote:
> > testutils/ptest2/conf/deployed/master-mr2.properties
> > Line 75 (original), 75 (patched)
> > <https://reviews.apache.org/r/67023/diff/1/?file=2018301#file2018301line75>
> >
> >     have you manually deployed these changes to the ptest server? this file is just a copy of whats already been deployed, so its just for reference
> >     
> >     also, why skip batching?
> 
> Andrew Sherman wrote:
>     OK I have no idea what ut.itests.qtest.skipBatching means I just copied TestEncryptedHDFSCliDriver :-(
>     
>     As for deploying these changes, I don't know what that means, my new tests did appear to run. can you explain more?

anytime you add a new cli driver, you have to manually modify a file on the ptest master server, you have to modify the file `/usr/local/hiveptest/profiles/master-mr2.properties` you probably don't have permissions though, so let me know the final diff for the this file and I can deploy it.

can we check if batching can be used for this? i think batching means that q-tests get bundled together into a "batch" of q-tests that are run in a single `mvn test` command


- Sahil


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67023/#review202831
-----------------------------------------------------------


On May 11, 2018, 11:38 p.m., Andrew Sherman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67023/
> -----------------------------------------------------------
> 
> (Updated May 11, 2018, 11:38 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> TestErasureCodingHDFSCliDriver uses a test-only CommandProcessor "ErasureProcessor"
> which allows .q files to contain Erasure Coding commands similar to those provided
> by the hdfs ec command
> https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-hdfs/HDFSErasureCoding.html.
> The Erasure Coding functionality is exposed through a new shim "HdfsFileErasureCodingPolicy".
> At this stage there are two .q files:
> erasure_commnds.q (a simple test to show ERASURE commands can run on local fs via
> TestCliDriver or on hdfs via TestErasureCodingHDFSCliDriver), and
> erasure_simple.q (which does some trivial queries to demonstrate basic functionality).
> More tests will come in future commits.
> 
> 
> Diffs
> -----
> 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestErasureCodingHDFSCliDriver.java PRE-CREATION 
>   itests/src/test/resources/testconfiguration.properties cf6d19a5937c3f4a82e4ffe09201af8a79da2e3d 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 6628336807b06cab49063673be0d8e9c5b5a7101 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 750fc69c5f210ca8f7bfe81b82ee9e3001fc07ba 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java 3d47991b603c94c8da2106e67192c8513ef783a7 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/ErasureProcessor.java PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/HiveCommand.java 56c7516ecfaf2421b0f3d3a188d05f38715b25b2 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 9f65a771f95a7c0bd3fdb4e56e47c0fc70235850 
>   ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java de43c2866f64e2ed5c74eab450de28f1a79248dc 
>   ql/src/test/queries/clientpositive/erasure_commands.q PRE-CREATION 
>   ql/src/test/queries/clientpositive/erasure_simple.q PRE-CREATION 
>   ql/src/test/results/clientpositive/erasure_commands.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_commands.q.out PRE-CREATION 
>   ql/src/test/results/clientpositive/erasurecoding/erasure_simple.q.out PRE-CREATION 
>   shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java ec06a88dc21346473bec6589c703167d50e3b367 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java b89081761780bf1f305d0196bb94bb0b54f7184f 
>   testutils/ptest2/conf/deployed/master-mr2.properties 7edc307f85744d60d322ad8087164625677fc230 
> 
> 
> Diff: https://reviews.apache.org/r/67023/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>