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
> 
> Sahil Takiar wrote:
>     interesting, should we handle other cluster types like Spark or MR too?

looks like it does already


> 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.
> 
> Sahil Takiar wrote:
>     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

deleted


> 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
> 
> Sahil Takiar wrote:
>     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?

Yes that is the use of NoopHdfsErasureCodingShim. I think you always need 2 implementations of the interface


> 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.
> 
> Sahil Takiar wrote:
>     see comment above about removing the cache

OK I have removed the cache and it does make the code simpler


- Andrew


-----------------------------------------------------------
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
> 
>