You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <an...@apache.org> on 2017/05/23 20:20:50 UTC
Re: Review Request 59428: Moved a couple of logging lines to
`WARNING` level.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59428/
-----------------------------------------------------------
(Updated May 23, 2017, 8:20 p.m.)
Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
Changes
-------
Review comments from Till.
Summary (updated)
-----------------
Moved a couple of logging lines to `WARNING` level.
Repository: mesos
Description
-------
This ensures ease of debugging when a HTTP request fails due to
access permissions (4xx errors) or when the path is not found.
Diffs (updated)
-----
3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c
Diff: https://reviews.apache.org/r/59428/diff/2/
Changes: https://reviews.apache.org/r/59428/diff/1-2/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 59428: Moved a couple of logging lines to
`WARNING` level.
Posted by Anand Mazumdar <an...@apache.org>.
> On May 23, 2017, 9:31 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 3787-3788 (original), 3787-3789 (patched)
> > <https://reviews.apache.org/r/59428/diff/2/?file=1730084#file1730084line3787>
> >
> > Is this being double logged?
> >
> > https://github.com/apache/mesos/blob/1.3.x/3rdparty/libprocess/src/process.cpp#L1477-L1482
>
> Anand Mazumdar wrote:
> Good catch! Let me remove this line in a follow up review in the chain and limit this review to just logging the `404` at the `WARNING` log level.
Added the review to remove the double logging earlier in the chain here: https://reviews.apache.org/r/59504/
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59428/#review175856
-----------------------------------------------------------
On May 23, 2017, 9:57 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59428/
> -----------------------------------------------------------
>
> (Updated May 23, 2017, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This ensures ease of debugging when a HTTP request fails due to
> access permissions (4xx errors) or when the path is not found.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c
>
>
> Diff: https://reviews.apache.org/r/59428/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 59428: Moved a couple of logging lines to
`WARNING` level.
Posted by Anand Mazumdar <an...@apache.org>.
> On May 23, 2017, 9:31 p.m., Benjamin Mahler wrote:
> > Logging from libprocess seems ok at this point, although ideally the user of libprocess could configure the logging independently from their own code.
> >
> > I think the old strategy was, roughly, to use 1 for error, 2 for warning and 3 for info. So so it would be great to take a little more time and do a sweep of process.cpp logging, rather than touching these two cases and moving on, not that that's what you were planning to do :)
I noticed this due to a user reported issue around them not being able to ascertain why their request failed without bumping up the logging level and hardly had any intentions of doing an audit of `process.cpp` or libprocess logging in general :-). I can file a follow up issue to do an audit if that helps or I am open to any suggestions you might have?
> On May 23, 2017, 9:31 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 3787-3788 (original), 3787-3789 (patched)
> > <https://reviews.apache.org/r/59428/diff/2/?file=1730084#file1730084line3787>
> >
> > Is this being double logged?
> >
> > https://github.com/apache/mesos/blob/1.3.x/3rdparty/libprocess/src/process.cpp#L1477-L1482
Good catch! Let me remove this line in a follow up review in the chain and limit this review to just logging the `404` at the `WARNING` log level.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59428/#review175856
-----------------------------------------------------------
On May 23, 2017, 8:20 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59428/
> -----------------------------------------------------------
>
> (Updated May 23, 2017, 8:20 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This ensures ease of debugging when a HTTP request fails due to
> access permissions (4xx errors) or when the path is not found.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c
>
>
> Diff: https://reviews.apache.org/r/59428/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 59428: Moved a couple of logging lines to
`WARNING` level.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59428/#review175856
-----------------------------------------------------------
Logging from libprocess seems ok at this point, although ideally the user of libprocess could configure the logging independently from their own code.
I think the old strategy was, roughly, to use 1 for error, 2 for warning and 3 for info. So so it would be great to take a little more time and do a sweep of process.cpp logging, rather than touching these two cases and moving on, not that that's what you were planning to do :)
3rdparty/libprocess/src/process.cpp
Lines 3787-3788 (original), 3787-3789 (patched)
<https://reviews.apache.org/r/59428/#comment249185>
Is this being double logged?
https://github.com/apache/mesos/blob/1.3.x/3rdparty/libprocess/src/process.cpp#L1477-L1482
- Benjamin Mahler
On May 23, 2017, 8:20 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59428/
> -----------------------------------------------------------
>
> (Updated May 23, 2017, 8:20 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This ensures ease of debugging when a HTTP request fails due to
> access permissions (4xx errors) or when the path is not found.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c
>
>
> Diff: https://reviews.apache.org/r/59428/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 59428: Moved a couple of logging lines to
`WARNING` level.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59428/#review175845
-----------------------------------------------------------
Ship it!
I meant `LOG()` in general. But looks like we do `LOG(WARNING)` more than `LOG(INFO)` in libprocess.
- Vinod Kone
On May 23, 2017, 8:20 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59428/
> -----------------------------------------------------------
>
> (Updated May 23, 2017, 8:20 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This ensures ease of debugging when a HTTP request fails due to
> access permissions (4xx errors) or when the path is not found.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c
>
>
> Diff: https://reviews.apache.org/r/59428/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 59428: Moved a couple of logging lines to
`WARNING` level.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59428/#review175915
-----------------------------------------------------------
Patch looks great!
Reviews applied: [59504, 59428]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 24, 2017, 5:57 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59428/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 5:57 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This ensures ease of debugging when a HTTP request fails due to
> access permissions (4xx errors) or when the path is not found.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c
>
>
> Diff: https://reviews.apache.org/r/59428/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 59428: Moved a couple of logging lines to
`WARNING` level.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59428/#review176490
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On May 23, 2017, 9:57 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59428/
> -----------------------------------------------------------
>
> (Updated May 23, 2017, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This ensures ease of debugging when a HTTP request fails due to
> access permissions (4xx errors) or when the path is not found.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c
>
>
> Diff: https://reviews.apache.org/r/59428/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 59428: Moved a couple of logging lines to
`WARNING` level.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59428/#review176491
-----------------------------------------------------------
Bad patch!
Reviews applied: [59504, 59428]
Failed command: ['cmd', '/c', 'support\\windows-build.bat 2>&1 > build_59428']
Error:
C:\Users\mesos\mesos>REM Licensed to the Apache Software Foundation (ASF) under one
C:\Users\mesos\mesos>REM or more contributor license agreements. See the NOTICE file
C:\Users\mesos\mesos>REM distributed with this work for additional information
C:\Users\mesos\mesos>REM regarding copyright ownership. The ASF licenses this file
C:\Users\mesos\mesos>REM to you under the Apache License, Version 2.0 (the
C:\Users\mesos\mesos>REM "License"); you may not use this file except in compliance
C:\Users\mesos\mesos>REM with the License. You may obtain a copy of the License at
C:\Users\mesos\mesos>REM
C:\Users\mesos\mesos>REM http://www.apache.org/licenses/LICENSE-2.0
C:\Users\mesos\mesos>REM
C:\Users\mesos\mesos>REM Unless required by applicable law or agreed to in writing, software
C:\Users\mesos\mesos>REM distributed under the License is distributed on an "AS IS" BASIS,
C:\Users\mesos\mesos>REM WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
C:\Users\mesos\mesos>REM See the License for the specific language governing permissions and
C:\Users\mesos\mesos>REM limitations under the License.
C:\Users\mesos\mesos>REM NOTE: Before you run this script, you must have the Visual Studio
C:\Users\mesos\mesos>REM environment variables set up. Visual Studio provides a script to do this,
C:\Users\mesos\mesos>REM depending on the version of Visual Studio installed:
C:\Users\mesos\mesos>REM /path/to/Visual Studio 14/VC/vcvarsall.bat
C:\Users\mesos\mesos>REM /path/to/Microsoft Visual Studio/2017/Community/VC/Auxiliary/Build/vcvars64.bat
C:\Users\mesos\mesos>REM NOTE: Batch doesn't have any way of exiting upon failing a command.
C:\Users\mesos\mesos>REM The best we can do is add this line after every command that matters:
C:\Users\mesos\mesos>REM if 0 neq 0 exit /b 0
C:\Users\mesos\mesos>REM NOTE: In order to run the tests, your Windows volume must have a
C:\Users\mesos\mesos>REM folder named `tmp` at the top level. You can create it like this:
C:\Users\mesos\mesos>REM MKDIR C:\tmp
C:\Users\mesos\mesos>REM Make sure that we are in the right directory. We do this by checking that
C:\Users\mesos\mesos>REM the `support` folder exists in the current directory and is not a symlink.
C:\Users\mesos\mesos>REM This code is awkwardly split across two conditionals because batch scripts
C:\Users\mesos\mesos>REM do not support logical operators like `&&`.
C:\Users\mesos\mesos>if not exist support (goto not_in_root )
C:\Users\mesos\mesos>fsutil reparsepoint query "support" | find "Symbolic Link" 1>nul && (goto not_in_root )
C:\Users\mesos\mesos>REM Create a build directory.
C:\Users\mesos\mesos>MKDIR build
C:\Users\mesos\mesos>CD build
C:\Users\mesos\mesos\build>if 0 NEQ 0 exit /b 0
C:\Users\mesos\mesos\build>REM Generate the Visual Studio solution.
C:\Users\mesos\mesos\build>REM You can pass in other flags by setting `OTHER_CMAKE_OPTIONS` before
C:\Users\mesos\mesos\build>REM calling the script. For example, the ASF CI will add `-DPATCHEXE_PATH=...`
C:\Users\mesos\mesos\build>REM because the path to GNU Patch is not the default.
C:\Users\mesos\mesos\build>if not defined CMAKE_GENERATOR (set CMAKE_GENERATOR=Visual Studio 15 2017 Win64 )
C:\Users\mesos\mesos\build>cmake .. -G "Visual Studio 15 2017 Win64" -T "host=x64" -DENABLE_LIBEVENT=1 -DHAS_AUTHENTICATION=0
-- The C compiler identification is MSVC 19.10.25017.0
-- The CXX compiler identification is MSVC 19.10.25017.0
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/BuildTools/VC/Tools/MSVC/14.10.25017/bin/HostX64/x64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/BuildTools/VC/Tools/MSVC/14.10.25017/bin/HostX64/x64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/BuildTools/VC/Tools/MSVC/14.10.25017/bin/HostX64/x64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/BuildTools/VC/Tools/MSVC/14.10.25017/bin/HostX64/x64/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- cotire 1.7.9 loaded.
-- ************************************************************
-- ********* Beginning Mesos CMake configuration step *********
-- ************************************************************
-- INSTALLATION PREFIX: C:/Program Files/Mesos
-- MACHINE SPECS:
-- Hostname:
-- OS: WINDOWS(10.0.14393)
-- Arch: AMD64
-- BitMode:
-- BuildID:
-- ************************************************************
-- Performing Test COMPILER_SUPPORTS_CXX11
-- Performing Test COMPILER_SUPPORTS_CXX11 - Success
-- GnuWin32 patch.exe exists at: C:/Program Files (x86)/GnuWin32/bin/patch.exe
-- Looking for pthread.h
-- Looking for pthread.h - not found
-- Found Threads: TRUE
-- CXX target mesos-1.4.0 cotired.
-- Configuring incomplete, errors occurred!
See also "C:/Users/mesos/mesos/build/CMakeFiles/CMakeOutput.log".
See also "C:/Users/mesos/mesos/build/CMakeFiles/CMakeError.log".
C:\Users\mesos\mesos\build>if 1 NEQ 0 exit /b 1
Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/console
- Mesos Reviewbot Windows
On May 23, 2017, 9:57 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59428/
> -----------------------------------------------------------
>
> (Updated May 23, 2017, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This ensures ease of debugging when a HTTP request fails due to
> access permissions (4xx errors) or when the path is not found.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c
>
>
> Diff: https://reviews.apache.org/r/59428/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 59428: Moved a couple of logging lines to
`WARNING` level.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59428/
-----------------------------------------------------------
(Updated May 23, 2017, 9:57 p.m.)
Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
Changes
-------
Review comments.
Repository: mesos
Description
-------
This ensures ease of debugging when a HTTP request fails due to
access permissions (4xx errors) or when the path is not found.
Diffs (updated)
-----
3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c
Diff: https://reviews.apache.org/r/59428/diff/3/
Changes: https://reviews.apache.org/r/59428/diff/2-3/
Testing
-------
make check
Thanks,
Anand Mazumdar