Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review141483 --- Closing this review due to inactivity. Please see our [guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md) for reopening reviews. - Joris Van Remoortere On July 7, 2015, 7:43 p.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34142/ > --- > > (Updated July 7, 2015, 7:43 p.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Discovers image(s), fetches to the image store, then provisions using > a backend. > > > Diffs > - > > include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 > src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 > src/slave/containerizer/mesos/containerizer.cpp > 8c102fb7d1f79ee768cb06de3a976ea12f958712 > src/slave/containerizer/provisioners/appc.hpp PRE-CREATION > src/slave/containerizer/provisioners/appc.cpp PRE-CREATION > src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b > src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 > > Diff: https://reviews.apache.org/r/34142/diff/ > > > Testing > --- > > > Thanks, > > Ian Downes > >
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review97499 --- This is no longer valid right? Can we discard all ian's patches? - Timothy Chen On July 7, 2015, 7:43 p.m., Ian Downes wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34142/ > --- > > (Updated July 7, 2015, 7:43 p.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Discovers image(s), fetches to the image store, then provisions using > a backend. > > > Diffs > - > > include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 > src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 > src/slave/containerizer/mesos/containerizer.cpp > 8c102fb7d1f79ee768cb06de3a976ea12f958712 > src/slave/containerizer/provisioners/appc.hpp PRE-CREATION > src/slave/containerizer/provisioners/appc.cpp PRE-CREATION > src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b > src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 > > Diff: https://reviews.apache.org/r/34142/diff/ > > > Testing > --- > > > Thanks, > > Ian Downes > >
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review94969 --- src/slave/containerizer/provisioners/appc.cpp (lines 471 - 481) https://reviews.apache.org/r/34142/#comment149709 Will move this into the bind mount backend. - Jiang Yan Xu On July 7, 2015, 12:43 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review91503 --- src/slave/containerizer/mesos/containerizer.cpp (line 65) https://reviews.apache.org/r/34142/#comment144997 Probably left over from a previous review? src/slave/containerizer/provisioners/appc.hpp (line 54) https://reviews.apache.org/r/34142/#comment144958 Who outside appc.cpp uses it? src/slave/containerizer/provisioners/appc.hpp (line 56) https://reviews.apache.org/r/34142/#comment144959 I see that this is used by Store. Possible to move to the Store base class to break circular dependency? src/slave/containerizer/provisioners/appc.hpp (lines 58 - 61) https://reviews.apache.org/r/34142/#comment144938 Looks like this is only used in discovery. Can it be moved to Discovery so we can break the Dicovery - Appc - Discovery review dependency? src/slave/containerizer/provisioners/appc.hpp (lines 63 - 67) https://reviews.apache.org/r/34142/#comment144939 Move to cpp if this is only used by appc.cpp? src/slave/containerizer/provisioners/appc.hpp (lines 64 - 67) https://reviews.apache.org/r/34142/#comment144940 Indentation. src/slave/containerizer/provisioners/appc.hpp (line 93) https://reviews.apache.org/r/34142/#comment144933 explicit src/slave/containerizer/provisioners/appc.hpp (line 101) https://reviews.apache.org/r/34142/#comment144936 Move to cpp? src/slave/containerizer/provisioners/appc.hpp (line 125) https://reviews.apache.org/r/34142/#comment145668 According the spec the ordering is significant so we better document it here. In fact the implementation does return the list of image in **topological** order (dependencies go before dependents is the list), which is great. It doesn't address duplicates though, which is something we can improve. src/slave/containerizer/provisioners/appc.hpp (line 127) https://reviews.apache.org/r/34142/#comment145221 s/hash/id/ ? As I commented on another review, I think it's less error-prone if we consistently use `id` rather than `hash`. src/slave/containerizer/provisioners/appc.hpp (line 132) https://reviews.apache.org/r/34142/#comment145222 s/hash/id/ ? src/slave/containerizer/provisioners/appc.hpp (line 148) https://reviews.apache.org/r/34142/#comment145208 What are additional fileds you imagine may be added in the future? src/slave/containerizer/provisioners/appc.cpp (line 56) https://reviews.apache.org/r/34142/#comment144957 Such as checking `if(acKind == ImageManifest)`? src/slave/containerizer/provisioners/appc.cpp (line 84) https://reviews.apache.org/r/34142/#comment145514 IIUC about the spec there is not a canonical name but this is a template for SimpleDiscovery. e.g. MetaDiscovery, if one were to implement it, doesn't follow this. Can we move it to SimpleDisovery and make LocalDiscovery (which may be more aptly named LocalSimpleDiscovery?) a subclass of SimpleDiscovery? src/slave/containerizer/provisioners/appc.cpp (line 113) https://reviews.apache.org/r/34142/#comment145519 Given the TODO above I am not sure if we still should do this at all. Maybe it's sufficient to just fill in the missing arch label and trust the operator will do the right thing? src/slave/containerizer/provisioners/appc.cpp (lines 287 - 288) https://reviews.apache.org/r/34142/#comment145564 Inline this? Moreover, with the path manipulation getting hairy in the code (different places remove and append rootfs) I think we'd better do something akin to **slave/paths.hpp**. src/slave/containerizer/provisioners/appc.cpp (lines 322 - 323) https://reviews.apache.org/r/34142/#comment145930 This is probably where the lambda becomes to long and a callback with a descriptive name (the old style) is better? src/slave/containerizer/provisioners/appc.cpp (lines 342 - 343) https://reviews.apache.org/r/34142/#comment145762 We typically break after the operator I think :) (e.g. https://github.com/apache/mesos/blob/bfe6c07b79550bb3d1f2ab6f5344d740e6eb6f60/3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp#L494) src/slave/containerizer/provisioners/appc.cpp (lines 349 - 351) https://reviews.apache.org/r/34142/#comment145253 2 spaces src/slave/containerizer/provisioners/appc.cpp (lines 365 - 375) https://reviews.apache.org/r/34142/#comment145464 Indentation: it should be two spaces right of the dot on `.then()`. src/slave/containerizer/provisioners/appc.cpp (line 409) https://reviews.apache.org/r/34142/#comment145231 Misaligned. src/slave/containerizer/provisioners/appc.cpp (line 423) https://reviews.apache.org/r/34142/#comment145463 fetchAll() already recursively traverses the dependency graph right? src/slave/containerizer/provisioners/appc.cpp (line 424)
Re: Review Request 34142: AppC provisioner.
On July 2, 2015, 1:48 a.m., Timothy Chen wrote: include/mesos/mesos.proto, line 1300 https://reviews.apache.org/r/34142/diff/2/?file=989783#file989783line1300 I believe we discussed this, but different acVersion will most likely have different schema. Unless we intend to only support one version (or anything that's backward compatible), we should version the protobuf too. Timothy Chen wrote: Actually at this point seems like AppC image spec most likely won't have a new version with OCP, so perhaps we don't have to. I'll let you decide :) We should at least say in the comment what acVersion this proto definiton is based on. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review90204 --- On July 7, 2015, 12:43 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34142: AppC provisioner.
On July 1, 2015, 5:40 p.m., Lily Chen wrote: src/slave/containerizer/provisioners/appc.cpp, lines 143-151 https://reviews.apache.org/r/34142/diff/2/?file=989787#file989787line143 What if the candidate is over-specified (has more labels)? Should this still be a match? According to the spec it should be a match. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review90173 --- On July 7, 2015, 12:43 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review90204 --- include/mesos/mesos.proto (line 1300) https://reviews.apache.org/r/34142/#comment143208 I believe we discussed this, but different acVersion will most likely have different schema. Unless we intend to only support one version (or anything that's backward compatible), we should version the protobuf too. - Timothy Chen On June 22, 2015, 4:57 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated June 22, 2015, 4:57 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review90173 --- src/slave/containerizer/provisioners/appc.cpp (lines 143 - 151) https://reviews.apache.org/r/34142/#comment143167 What if the candidate is over-specified (has more labels)? Should this still be a match? src/slave/containerizer/provisioners/appc.cpp (line 203) https://reviews.apache.org/r/34142/#comment143168 Maybe generalize the Failure message to something like Provisioner and Image type mismatch src/slave/containerizer/provisioners/appc.cpp (lines 484 - 487) https://reviews.apache.org/r/34142/#comment143174 Do we want to remove everything on a failure? This could make finding the failure point difficult. - Lily Chen On June 22, 2015, 4:57 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated June 22, 2015, 4:57 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated June 22, 2015, 9:57 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- - Use protobuf representation of Appc Image Manifest - Implemented recover Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs (updated) - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34142: AppC provisioner.
On May 18, 2015, 3:20 p.m., Chi Zhang wrote: src/slave/containerizer/provisioners/appc.hpp, line 50 https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line50 nit: return type could be const? Why? I don't see what it would add? On May 18, 2015, 3:20 p.m., Chi Zhang wrote: src/slave/containerizer/provisioners/appc.hpp, lines 60-62 https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line60 If we say the dependency itself is an AppcImage that has n AppcImage(dependencies). We can kill the dependency type? AppcImage could then recursively resolve the dependencies on its own, using the composite pattern described above, i.e., AppcProvisionerProcess simply needs to call rootImage.resolve(), who loops over dependent images and calls image.resolve(). We can then have fetch, fetchDependencies (not needed anymore), fetchURIs and match togerther go into AppcImage::resolve. AppcImage will now need functionality provided by discover and store; AppcPP only uses backend and does mounts. The distinction is that AppcImage::Dependency is something that is desired, and is specified by a name plus possibly labels and a hash, but has not been realized (fetched). An AppcImage only refers to an actual image that has been fetched and its properties come from the actual image data (extracted from the json manifest and or by computing the hash). AppcImage(::Dependency) is meant to be a simple data structure, the coordination of the discovery, fetch and store is coordinated by the provisioner. On May 18, 2015, 3:20 p.m., Chi Zhang wrote: src/slave/containerizer/provisioners/appc.hpp, line 101 https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line101 nit: newline after ',' std::string, std::string are the template parameters for the hashmap, i.e., the whole thing forms the type for labels. On May 18, 2015, 3:20 p.m., Chi Zhang wrote: src/slave/containerizer/provisioners/appc.hpp, lines 74-75 https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line74 const-able? Code refactored to remove this. On May 18, 2015, 3:20 p.m., Chi Zhang wrote: src/slave/containerizer/provisioners/appc.hpp, lines 135-139 https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line135 some of these should be const? Code refactored to remove this. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/#review84199 --- On May 12, 2015, 5:48 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated May 12, 2015, 5:48 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs - src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34142: AppC provisioner.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review84199
---
src/slave/containerizer/mesos/containerizer.cpp
https://reviews.apache.org/r/34142/#comment135361
can tweak the constructors so that the containerizer doesn't need to be
exposed to ProvisionerProcesses, which is implementation detail?
src/slave/containerizer/provisioners/appc.hpp
https://reviews.apache.org/r/34142/#comment135339
nit: return type could be const?
src/slave/containerizer/provisioners/appc.hpp
https://reviews.apache.org/r/34142/#comment135375
If we say the dependency itself is an AppcImage that has n
AppcImage(dependencies). We can kill the dependency type?
AppcImage could then recursively resolve the dependencies on its own, using
the composite pattern described above, i.e., AppcProvisionerProcess simply
needs to call rootImage.resolve(), who loops over dependent images and calls
image.resolve().
We can then have fetch, fetchDependencies (not needed anymore), fetchURIs
and match togerther go into AppcImage::resolve. AppcImage will now need
functionality provided by discover and store; AppcPP only uses backend and does
mounts.
src/slave/containerizer/provisioners/appc.hpp
https://reviews.apache.org/r/34142/#comment135340
const-able?
src/slave/containerizer/provisioners/appc.hpp
https://reviews.apache.org/r/34142/#comment135341
nit: newline after ','
src/slave/containerizer/provisioners/appc.hpp
https://reviews.apache.org/r/34142/#comment135342
nit: craft a list and then return -.join(list)?
src/slave/containerizer/provisioners/appc.hpp
https://reviews.apache.org/r/34142/#comment135343
some of these should be const?
src/slave/containerizer/provisioners/appc.hpp
https://reviews.apache.org/r/34142/#comment135358
nit: const-able?
src/slave/containerizer/provisioners/appc.cpp
https://reviews.apache.org/r/34142/#comment135360
Is it possible to tweak the store{process}'s constructors to hide the
implemention detail of there existing a StoreProcess?
- Chi Zhang
On May 13, 2015, 12:48 a.m., Ian Downes wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/
---
(Updated May 13, 2015, 12:48 a.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Repository: mesos
Description
---
Discovers image(s), fetches to the image store, then provisions using
a backend.
Diffs
-
src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
src/slave/containerizer/mesos/containerizer.cpp
b644b9c74bc23cf78c0a53284544be6cdaef2f8a
src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
Diff: https://reviews.apache.org/r/34142/diff/
Testing
---
Thanks,
Ian Downes
