Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============4249404938341174202=="
MIME-Version: 1.0
Subject: Re: Review Request 47040: Monitor changes to FairScheduler.xml file
 and automatically update / validate jobs submitted to fair-scheduler
From: Reuben Kuhnert <sircodesalot@gmail.com>
To: Sergio Pena <sergio.pena@cloudera.com>, Lenni Kuff <lskuff@cloudera.com>,
 Mohit Sabharwal <mohit@cloudera.com>
Cc: Reuben Kuhnert <sircodesalot@gmail.com>, hive <dev@hive.apache.org>
Date: Wed, 11 May 2016 19:17:32 -0000
Message-ID: <20160511191732.1967.62749@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: Reuben Kuhnert <noreply@reviews.apache.org>
References: <20160511022307.1967.72922@reviews.apache.org>
In-Reply-To: <20160511022307.1967.72922@reviews.apache.org>
Reply-To: Reuben Kuhnert <sircodesalot@gmail.com>
archived-at: Wed, 11 May 2016 19:17:35 -0000

--===============4249404938341174202==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java, line 502
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378910#file1378910line502>
> >
> >     Looks like you have the same conditional check both here and in the function you're calling (validateYarnQueue). In affect, we're doing the same check twice. Remove one ?

This is an assertion. It's exposed externally so callers can check when to perform this operation, but if called in an invalid state, we should fail. Ideally, an 'Optional' with a callback would be better though. C'est la vie.


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java, line 136
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378913#file1378913line136>
> >
> >     Should this be synchronized at all ? The event is not shared state, right ? Are we mutating any shared state in this method ?
> >     
> >     Also, make it private ?

'this.callbacks' can be modified from another thread if this function isn't monitored. Also, it's made protected for testing purposes (though if there's a better practice for this, let me know - not hyper familiar with Java).


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java, line 64
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378917#file1378917line64>
> >
> >     why synchronized ? Didn't see shared share getting modified anywhere...

Yeah, originally the file-system-watcher was static final (to ensure that if multiple instances of the shim exist for some reason, the configuration across them all is the same - and callbacks aren't fired multiple times). I ultimately decided against this though - this is just a legacy of that failed effort.


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java, lines 74-76
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378913#file1378913line74>
> >
> >     This seems strange.
> >     
> >     Why are we closing the watchService() and creating a new one here?
> >     
> >     Can the existing watchService() watch more files?
> >     
> >     Add comments explaining why this is necessary.

Overuse of 'functional'-style (Prefer new immutable objects over modifying existing ones, aka "state is evil"). This introduces a suble bug though if the file is modified while the watcher is being re-generated. This can't be helped if we're removing a watch though (since there's not 'unregister' function).


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java, line 125
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378913#file1378913line125>
> >
> >     Should this be a warning ?
> >     
> >     Also, add the exception to the log.
> >     
> >     LOG.warn("..." + ex, ex)
> >     
> >     This catch block also doesn't have the close() call. Need a finally block?

A finally would close the service after each iteration. But I modified everything else.


> On May 11, 2016, 2:23 a.m., Mohit Sabharwal wrote:
> > shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java, line 125
> > <https://reviews.apache.org/r/47040/diff/3/?file=1378917#file1378917line125>
> >
> >     rename to getConfigForUser(...)
> >     
> >     Also, not quiet sure why this needs to be synchronized.

The configuration resolvers are cached. But the cache is cleared if the location of 'fair-scheduler.xml' (YARN_SCHEDULER_FILE_PROPERTY) changes, or is modified. No good if one thread clears the cache while another is reading it.


- Reuben


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


On May 10, 2016, 10:54 p.m., Reuben Kuhnert wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47040/
> -----------------------------------------------------------
> 
> (Updated May 10, 2016, 10:54 p.m.)
> 
> 
> Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.
> 
> 
> Bugs: HIVE-13696
>     https://issues.apache.org/jira/browse/HIVE-13696
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Ensure that jobs sent to YARN with impersonation off are correctly routed to the proper queue based on fair-scheduler.xml. Monitor this file for changes and validate that jobs can only be sent to queues authorized for the user.
> 
> 
> Diffs
> -----
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 926f6e8 
>   ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java a0015eb 
>   shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java PRE-CREATION 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 63803b8 
>   shims/scheduler/pom.xml b36c123 
>   shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java PRE-CREATION 
>   shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java 372244d 
>   shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java PRE-CREATION 
>   shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47040/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Reuben Kuhnert
> 
>


--===============4249404938341174202==--