Mailing-List: contact dev-help@impala.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.apache.org
MIME-Version: 1.0
References: <CAEoRBewDA=AY0=iDngPWKqSArU10KhbGzj4YBD2DrZhxpJR2qQ@mail.gmail.com>
 <CAGcSVAc2RW6WDCVYiFCL70+c83KRFsiMYqxqKkjs+JRUEcG7ew@mail.gmail.com>
 <CAGcSVAc1Z2Ldkd=MNWdMbG3BN6sOdrcBf4kPft30CRbdwtAq0Q@mail.gmail.com>
 <CAD8O1-O9NRPj41b_agsdmxOuAGJPt18xJyg9dNzjen3honjFMA@mail.gmail.com>
 <CAKrh3XSTuSrGhnqEhRuqy9r+XXDt-=YH9SScH9ypeeOoFb8P4w@mail.gmail.com>
 <CAGcSVAeAPQd4szE3f4w2RdwWpkFMj97f1oh+P6yP=YSSCqjpZw@mail.gmail.com>
 <CAC-pSX0cj6GOfiW_dtG3FpEC8kxXFKsS71osvAAsVyh0PzxzTg@mail.gmail.com>
 <CAGcSVAfm-jhT4Qf4F0qy+2SHhnH0xQ04wh-y3mvVp-Vskgd7Bg@mail.gmail.com>
 <CAGcSVAdH3o4qu-TEgy0H1H0ZOYeb4QDLt=tpGkp+_p3TJqqf0w@mail.gmail.com>
 <CAAzK7q8sQ2viGNp0VJW4xXtNchffcPPtfb+LhGrrJ++hSuOr7A@mail.gmail.com> <CAGcSVAdgyNTFzO5pOuWaEOnQCyTR7B4jmqSuXq3hashhB1X__g@mail.gmail.com>
In-Reply-To: <CAGcSVAdgyNTFzO5pOuWaEOnQCyTR7B4jmqSuXq3hashhB1X__g@mail.gmail.com>
From: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Date: Wed, 03 Jan 2018 19:43:28 +0000
Message-ID: <CAAzK7q-M2GkbLnH52gywtpiM=0qqbBCZWyTqVat-BrYd8mOB3g@mail.gmail.com>
Subject: Re: New Impala Contributors: IMPALA-6296
To: dev@impala.apache.org
Content-Type: multipart/alternative; boundary="001a113ed4f28e0d070561e46ec4"
archived-at: Wed, 03 Jan 2018 19:43:45 -0000

--001a113ed4f28e0d070561e46ec4
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 3, 2018 at 10:06 AM Manaswini Maharana <mmaharana@cloudera.com>
wrote:

> Thanks for the pointers, Thomas. I was able to run the Jenkin tests, but
> not sure how to manually add the links to Gerrit as recommended. Do I  ad=
d
> it through the reply text box as shown below?
>

Yes, you can just post it as a comment on the review using the "Reply"
button


>
> - Mansi
> =E2=80=8B
>
> On Tue, Jan 2, 2018 at 3:04 PM, Thomas Tauber-Marshall <
> tmarshall@cloudera.com> wrote:
>
>> On Fri, Dec 29, 2017 at 11:12 AM Manaswini Maharana <
>> mmaharana@cloudera.com>
>> wrote:
>>
>> > I've pushed the initial changes to -
>> > https://gerrit.cloudera.org/#/c/8923/
>> >
>> > Steps that I've followed to make this contribution - As this is my
>> first,
>> > I want to elaborate a little bit to ensure I'm covering the bases righ=
t.
>> >
>> > 1. After the following the extensive contribution guide
>>
> > <
>> https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impal=
a
>> >,
>
>
>> > setup the development environment on machine with Ubuntu 16.04 OS, 15 =
GB
>> > memory, Intel=C2=AE Core=E2=84=A2 i7-8550U CPU @ 1.80GHz =C3=97 8  & 5=
12 GB SSD
>> > 2. Modified the build process to avoiding building against -so
>> > (IMPALA-5365 & IMPALA-3926)
>> > 3. Reproduced the issue. Attached Impalad error log.
>> > 4. Put in the code fix - added a condition check to ensure
>> > 'emit_perf_map_' evaluates to true before the dependent
>> > object(perf_map_lock_) is asserted using DCHECK.
>> > 5. Unit tested the changes using Impala-shell. No daemon crash
>> encountered
>> > this time.
>> > 6. Loaded and ran the "be" unit test cases to ensure the change did no=
t
>> > break other stuff. Attached the results.
>> >
>> >
>> > Questions I have:
>> > 1. Is it recommened to run more end-to-end unit tests or does it depen=
d
>> on
>> > the intensity of the change ? Like here I've limited it to just runnin=
g
>> be
>> > tests, is that ok?
>> >
>>
>> In general, you're expected to run any tests that may reasonably be
>> affected by the change, and of course the more testing the better. This
>> change affects basically any query that uses codegen, which is a
>> significant portion of our tests, so running the full suite would probab=
ly
>> be a good idea (eg bin/run-all-tests.sh)
>>
>> You may also want to try out the Jenkins jobs that we provide for testin=
g
>> patches, as described here:
>>
>> https://cwiki.apache.org/confluence/display/IMPALA/Using+Gerrit+to+submi=
t+and+review+patches#UsingGerrittosubmitandreviewpatches-Verifyingapatch(op=
entoallImpalacontributors)
>>
>>
>> > 2. As per Tim's comment, I should see *.asm files in codegen-modules
>> after
>> > the fix, but I don't see any. Am I missing anything ?
>> >
>>
>> You may need to manually create the directory
>> $IMPALA_HOME/codegen-modules/
>>
>>
>> >
>> > Thanks & appreciate your input,
>> > Mansi
>> >
>> >
>> > On Fri, Dec 29, 2017 at 12:43 PM, Manaswini Maharana <
>> > mmaharana@cloudera.com> wrote:
>> >
>> >> Thank you!
>> >>
>> >> On Fri, Dec 29, 2017 at 11:47 AM, Jim Apple <jbapple@cloudera.com>
>> wrote:
>> >>
>> >>> Done
>> >>> On Fri, Dec 29, 2017 at 7:12 AM, Manaswini Maharana
>> >>> <mmaharana@cloudera.com> wrote:
>> >>> > username: mmaharana
>> >>> >
>> >>> > On Fri, Dec 29, 2017 at 10:07 AM, Jin Chul Kim <jinchul@gmail.com>
>> >>> wrote:
>> >>> >
>> >>> >> Hi,
>> >>> >>
>> >>> >> I guess she doesn't have a privilege to change assignee. The
>> >>> permission
>> >>> >> should be provided by somebody who has an admin privilege.
>> >>> >> @Mansi, please share your username.
>> >>> >>
>> >>> >> Best regards,
>> >>> >> Jinchul
>> >>> >>
>> >>> >> 2017-12-30 0:02 GMT+09:00 Vincent Tran <vttran@cloudera.com>:
>> >>> >>
>> >>> >> > You should be able to just simply "assign to me" if you are
>> signed
>> >>> into
>> >>> >> ADD
>> >>> >> > Jira.
>> >>> >> >
>> >>> >> > On Dec 29, 2017 9:54 AM, "Manaswini Maharana" <
>> >>> mmaharana@cloudera.com>
>> >>> >> > wrote:
>> >>> >> >
>> >>> >> > > Good morning, Team - any update on this?
>> >>> >> > >
>> >>> >> > > - Mansi
>> >>> >> > >
>> >>> >> > >
>> >>> >> > > On Thu, Dec 28, 2017 at 12:32 AM, Manaswini Maharana <
>> >>> >> > > mmaharana@cloudera.com
>> >>> >> > > > wrote:
>> >>> >> > >
>> >>> >> > > > Hello team, I'd like to contribute to Impala and would like
>> to
>> >>> start
>> >>> >> it
>> >>> >> > > > with this (IMPALA-6296) jira. Can I please have it assigned
>> to
>> >>> me?
>> >>> >> > > >
>> >>> >> > > > My development environment is ready. I'm on ubuntu 16.04 so
>> yes
>> >>> some
>> >>> >> > > > challenges to set up the environment especially with the
>> >>> >> > LD_LIBRARY_PATH
>> >>> >> > > > issues. So far I'm able to work past it and might ask for
>> >>> another
>> >>> >> pair
>> >>> >> > of
>> >>> >> > > > eyes to ensure it follows standard. The builds are going fi=
ne
>> >>> and
>> >>> >> runs
>> >>> >> > a
>> >>> >> > > > bit longer as I've disabled the -so. Apart from setup, I'm
>> was
>> >>> able
>> >>> >> to
>> >>> >> > > > reproduce the issue and might have been ready with the fix
>> >>> which I'd
>> >>> >> > like
>> >>> >> > > > to discuss  once I'm officially assigned the jira.
>> >>> >> > > >
>> >>> >> > > > Thanks Tim, for the instructions, it was very helpful.
>> >>> >> > > >
>> >>> >> > > >
>> >>> >> > > > Thanks!
>> >>> >> > > > Mansi
>> >>> >> > > >
>> >>> >> > > >
>> >>> >> > > >
>> >>> >> > > > On Tue, Dec 12, 2017 at 8:24 PM, Tim Armstrong <
>> >>> >> > tarmstrong@cloudera.com>
>> >>> >> > > > wrote:
>> >>> >> > > >
>> >>> >> > > >> If you'd like to contribute a patch to Impala, but aren't
>> sure
>> >>> what
>> >>> >> > > >> you want to work on, you can look at Impala's newbie issue=
s:
>> >>> >> > > >> https://issues.apache.org/jira/issues/?filter=3D12341668. =
You
>> >>> can
>> >>> >> find
>> >>> >> > > >> detailed instructions on submitting patches at
>> >>> >> > > >> https://cwiki.apache.org/confluence/display/IMPALA/
>> >>> >> > > Contributing+to+Impala
>> >>> >> > > >> .
>> >>> >> > > >> This is a walkthrough of a ticket a new contributor could
>> take
>> >>> on,
>> >>> >> > > >> with hopefully enough detail to get you going but not so
>> much
>> >>> to
>> >>> >> take
>> >>> >> > > >> away the fun.
>> >>> >> > > >>
>> >>> >> > > >> How can we solve
>> >>> https://issues.apache.org/jira/browse/IMPALA-6296,
>> >>> >> > > >> "DCheck in CodegenSymbolEmitter when --asm_module_dir is
>> set on
>> >>> >> debug
>> >>> >> > > >> build
>> >>> >> > > >> "?
>> >>> >> > > >> First, make sure you have your development environment set
>> up.
>> >>> Let's
>> >>> >> > > >> see if we can reproduce the issue.
>> >>> >> > > >>
>> >>> >> > > >> Create a directory for the codegen modules then start
>> >>> impala-server
>> >>> >> > > >> $ mkdir codegen-modules
>> >>> >> > > >> $ start-impala-cluster.py --impalad_args=3D"--opt_module_d
>> >>> >> > > >> ir=3Dcodegen-modules
>> >>> >> > > >> --unopt_module_dir=3Dcodegen-modules --asm_module_dir=3Dco=
degen-
>> >>> >> modules"
>> >>> >> > > >>
>> >>> >> > > >> Now run a query through Impala. I chose this query because
>> it
>> >>> uses
>> >>> >> > > >> Impala's
>> >>> >> > > >> runtime code generation ("codegen") to improve performance
>> >>> >> > > >> $ bin/impala-shell.sh -q "select count(*) from
>> tpch.lineitem"
>> >>> >> > > >>
>> >>> >> > > >> After a few seconds, you should see an error from Impala
>> >>> crashing:
>> >>> >> > > >> Error communicating with impalad: TSocket read 0 bytes
>> >>> >> > > >> Could not execute command: select count(*) from
>> tpch.lineitem
>> >>> >> > > >>
>> >>> >> > > >> Attempting to run any further queries will fail because of
>> the
>> >>> >> crashed
>> >>> >> > > >> Impala daemons:
>> >>> >> > > >> $ impala-shell.sh -q "select count(*) from tpch.lineitem";
>> >>> >> > > >> Starting Impala Shell without Kerberos authentication
>> >>> >> > > >> Error connecting: TTransportException, Could not connect t=
o
>> >>> >> > > >> localhost:21000
>> >>> >> > > >> Not connected to Impala, could not execute queries.
>> >>> >> > > >>
>> >>> >> > > >> If you look at logs/cluster/impalad.ERROR you will see a
>> >>> message
>> >>> >> > > >> explaining
>> >>> >> > > >> the crash - we hit a DCHECK (a.k.a. an assertion):
>> >>> >> > > >>
>> >>> >> > > >> F1212 17:10:07.642722  9138 codegen-symbol-emitter.cc:90]
>> Check
>> >>> >> > failed:
>> >>> >> > > >> perf_map_.find(obj.getData().data()) !=3D perf_map_.end()
>> >>> >> > > >> *** Check failure stack trace: ***
>> >>> >> > > >>     @          0x3be16ed  google::LogMessage::Fail()
>> >>> >> > > >>     @          0x3be2f92  google::LogMessage::SendToLog()
>> >>> >> > > >>     @          0x3be10c7  google::LogMessage::Flush()
>> >>> >> > > >>     @          0x3be468e  google::LogMessageFatal::~
>> >>> >> LogMessageFatal()
>> >>> >> > > >>     @          0x1b354f1
>> >>> >> > > >> impala::CodegenSymbolEmitter::NotifyFreeingObject()
>> >>> >> > > >>     @          0x375b791  llvm::MCJIT::NotifyFreeingObject=
()
>> >>> >> > > >>     @          0x375b811  llvm::MCJIT::~MCJIT()
>> >>> >> > > >>     @          0x375bfc9  llvm::MCJIT::~MCJIT()
>> >>> >> > > >>     @          0x1b265ba
>> std::default_delete<>::operator()()
>> >>> >> > > >>     @          0x1b23e21  std::unique_ptr<>::reset()
>> >>> >> > > >>     @          0x1b11198  impala::LlvmCodeGen::Close()
>> >>> >> > > >>     @          0x182194d
>> >>> impala::RuntimeState::ReleaseResources()
>> >>> >> > > >>     @          0x1893a35
>> >>> impala::FragmentInstanceState::Close()
>> >>> >> > > >>     @          0x1890d58
>> impala::FragmentInstanceState::Exec()
>> >>> >> > > >>     @          0x1879afa
>> impala::QueryState::ExecFInstance()
>> >>> >> > > >>     @          0x18783bc
>> >>> >> > > >> _ZZN6impala10QueryState15StartFInstancesEvENKUlvE_clEv
>> >>> >> > > >>     @          0x187a739
>> >>> >> > > >> _ZN5boost6detail8function26void_function_obj_invoker0IZN6i=
mp
>> >>> >> > > >> ala10QueryState15StartFInstancesEvEUlvE_vE6invokeERNS1_
>> >>> >> > > 15function_bufferE
>> >>> >> > > >>     @          0x17c7200  boost::function0<>::operator()()
>> >>> >> > > >>     @          0x1abdf8b  impala::Thread::SuperviseThread(=
)
>> >>> >> > > >>     @          0x1ac6b16
>> boost::_bi::list4<>::operator()<>()
>> >>> >> > > >>     @          0x1ac6a59  boost::_bi::bind_t<>::operator()=
()
>> >>> >> > > >>     @          0x1ac6a1c
>> boost::detail::thread_data<>::run()
>> >>> >> > > >>     @          0x2d6b08a  thread_proxy
>> >>> >> > > >>     @     0x7fcb5ff146ba  start_thread
>> >>> >> > > >>     @     0x7fcb5fc4a3dd  clone
>> >>> >> > > >>
>> >>> >> > > >> It looks like something is wrong with the code - 'perf_map=
_'
>> >>> doesn't
>> >>> >> > > have
>> >>> >> > > >> anything in it. If we look a few lines above, it looks lik=
e
>> >>> >> > 'perf_map_'
>> >>> >> > > is
>> >>> >> > > >> only filled in if 'emit_perf_map_' is true.
>> >>> >> > > >>
>> >>> >> > > >>   if (emit_perf_map_) {
>> >>> >> > > >>     lock_guard<SpinLock> perf_map_lock(perf_map_lock_);
>> >>> >> > > >>     DCHECK(perf_map_.find(obj.getData().data()) =3D=3D
>> >>> >> perf_map_.end());
>> >>> >> > > >>     perf_map_[obj.getData().data()] =3D
>> >>> std::move(perf_map_entries);
>> >>> >> > > >>     WritePerfMapLocked();
>> >>> >> > > >>   }
>> >>> >> > > >>
>> >>> >> > > >> Maybe we should also be checking 'emit_perf_map_' before t=
he
>> >>> DCHECK?
>> >>> >> > > >>
>> >>> >> > > >> After you have fixed this bug, you should be able to run t=
he
>> >>> query
>> >>> >> > > without
>> >>> >> > > >> Impala crashing and you should be able to inspect the
>> assembly
>> >>> >> > generated
>> >>> >> > > >> by
>> >>> >> > > >> Impala in codegen-modules/*.asm
>> >>> >> > > >>
>> >>> >> > > >> Have fun, and don't be afraid to ask dev@impala.apache.org
>> is
>> >>> you
>> >>> >> > have
>> >>> >> > > >> any questions!
>> >>> >> > > >>
>> >>> >> > > >
>> >>> >> > > >
>> >>> >> > >
>> >>> >> >
>> >>> >>
>> >>>
>> >>
>> >>
>> >
>>
>

--001a113ed4f28e0d070561e46ec4--