> On 5 Aug 2018, at 18:01, Bolke de Bruin <bdbr...@gmail.com> wrote: > > Hi Ash, > > Thanks a lot for the proper review, obviously I would have liked that these > issues (I think just one) are popping up at rc3 but I understand why it > happened.
Yeah, sorry I didn't couldn't make time to test the betas :( > > Can you work out a patch for the k8s issue? I’m sure Fokko and others can > chime in to make sure it will be the right change. Working on it - I'll go for a conditional import, and only except if the "k8s://" scheme is specified I think. > > On the timezone change. The database will do the right thing and correctly > transform a datetime into the timezone the client is using. Even then we > enforce UTC internally and only transform it for user interaction or when it > is relevant (to make sure we do daylight savings for example). It is > therefore not required to force a timezone setting with sql alchemy beyond > when we convert to timezone aware (see migration scripts). I think the database is being "smart" here in converting, but I'm not sure it's the Right thing. It wouldn't surprise me if we have other places in the codebase that expect datetime columns to come back in UTC, but they might come back in DB-server local timezone. Trying https://github.com/apache/incubator-airflow/compare/master...ashb:set-timezone-to-utc-on-connect?expand=1 <https://github.com/apache/incubator-airflow/compare/master...ashb:set-timezone-to-utc-on-connect?expand=1> - it "fixes" my logging issue, tests are running https://travis-ci.org/ashb/incubator-airflow/builds/412360920 > > On the logging file format I agree this could be handled better. However I do > think we should honor local system time for this as this is the standard for > any other logging. Also logging output will be time stamped in local system > time. Maybe we could cut off the timezone identifier as it can be assumed to > be in local system time (+01:00). The issue with just cutting off the timezone is that old log files are now unviewable - they ran at 00:00:00 UTC, but the hour of the record coming back is 01. > > If we take on the k8s fix we can also fix the logging format. What do you > think? Also as a quick fix I've changed the UPDATING.md as suggested: https://github.com/apache/incubator-airflow/compare/master...ashb:updating-for-logging-changes?expand=1. The log format is a bit clunky, but the note about log_task_reader is needed either way. (Do we need a Jira ticket for this sort of change, or is AIRFLOW-XXX okay for this?) > > Cheers > Bolke > > Verstuurd vanaf mijn iPad > >> Op 5 aug. 2018 om 18:13 heeft Ash Berlin-Taylor >> <ash_airflowl...@firemirror.com> het volgende geschreven: >> >> Relating to 2): I'm not sure that the upgrade from timezoneless to timezone >> aware colums in the task instance is right, or at least it's not what I >> expected. >> >> Before weren't all TZs from schedule dates etc in UTC? For the same task >> instance (these outputs from psql directly): >> >> before: execution_date=2017-09-04 00:00:00 >> after: execution_date=2017-09-04 01:00:00+01 >> >> **Okay the migration is fine**. It appears that the migration has done the >> right thing, but my local DB I'm testing with has a Timezone of GB set, so >> Postgres converts it to that TZ on returning an object. >> >> 3) Do we need to set the TZ of the connection to UTC in SQLAlchemy to have >> consistent behaviour? Is this possible some how? I don't know SQLAlchemy >> that well. >> >> >> -ash >> >>> On 5 Aug 2018, at 16:01, Ash Berlin-Taylor <ash_airflowl...@firemirror.com> >>> wrote: >>> >>> 1.) Missing UPDATING note about change of task_log_reader to now always >>> being "task" (was "s3.task" before.). Logging config is much simpler now >>> though. This may be particular to my logging config, but given how much of >>> a pain it was to set up S3 logging in 1.9 I have shared my config with some >>> people in the Gitter chat so It's not just me. >>> >>> 2) The path that log-files are written to in S3 has changed (again - this >>> happened from 1.8 to 1.9). I'd like to avoid having to move all of my log >>> files again to continue viewing them. The change is that the path now (in >>> 1.10) has a timezone in it, and the date is in local time, before it was >>> UTC: >>> >>> before: 2018-07-23T00:00:00/1.log >>> after: 2018-07-23T01:00:00+01:00/1.log >>> >>> We can possibly get away with an updating note about this to set a custom >>> log_filename_template. Testing this now. >>> >>>> On 5 Aug 2018, at 15:00, Ash Berlin-Taylor <a...@firemirror.com> wrote: >>>> >>>> -1(binding) from me. >>>> >>>> Installed with: >>>> >>>> AIRFLOW_GPL_UNIDECODE=yes pip install >>>> 'https://dist.apache.org/repos/dist/dev/incubator/airflow/1.10.0rc3/apache-airflow-1.10.0rc3+incubating-bin.tar.gz#egg=apache-airflow[emr >>>> >>>> <https://dist.apache.org/repos/dist/dev/incubator/airflow/1.10.0rc3/apache-airflow-1.10.0rc3+incubating-bin.tar.gz#egg=apache-airflow[emr>, >>>> s3, crypto]>=1.10' >>>> >>>> Install went fine. >>>> >>>> Our DAGs that use SparkSubmitOperator are now failing as there is now a >>>> hard dependency on the Kubernetes client libs, but the `emr` group doesn't >>>> mention this. >>>> >>>> Introduced in https://github.com/apache/incubator-airflow/pull/3112 >>>> <https://github.com/apache/incubator-airflow/pull/3112> >>>> >>>> I see two options for this - either conditionally enable k8s:// support if >>>> the import works, or (less preferred) add kube-client to the emr deps >>>> (which I like less) >>>> >>>> Sorry - this is the first time I've been able to test it. >>>> >>>> I will install this dep manually and continue testing. >>>> >>>> -ash >>>> >>>> (Normally no time at home due to new baby, but I got a standing desk, and >>>> a carrier meaning she can sleep on me and I can use my laptop. Win!) >>>> >>>> >>>> >>>>> On 4 Aug 2018, at 22:32, Bolke de Bruin <bdbr...@gmail.com >>>>> <mailto:bdbr...@gmail.com>> wrote: >>>>> >>>>> Bump. >>>>> >>>>> Committers please cast your vote. >>>>> >>>>> B. >>>>> >>>>> Sent from my iPhone >>>>> >>>>>> On 3 Aug 2018, at 13:23, Driesprong, Fokko <fo...@driesprong.frl >>>>>> <mailto:fo...@driesprong.frl>> wrote: >>>>>> >>>>>> +1 Binding >>>>>> >>>>>> Installed it using: SLUGIFY_USES_TEXT_UNIDECODE=yes pip install >>>>>> https://dist.apache.org/repos/dist/dev/incubator/airflow/1.10.0rc3/apache-airflow-1.10.0rc3+incubating-bin.tar.gz >>>>>> >>>>>> <https://dist.apache.org/repos/dist/dev/incubator/airflow/1.10.0rc3/apache-airflow-1.10.0rc3+incubating-bin.tar.gz> >>>>>> >>>>>> Cheers, Fokko >>>>>> >>>>>> 2018-08-03 9:47 GMT+02:00 Bolke de Bruin <bdbr...@gmail.com>: >>>>>> >>>>>>> Hey all, >>>>>>> >>>>>>> I have cut Airflow 1.10.0 RC3. This email is calling a vote on the >>>>>>> release, >>>>>>> which will last for 72 hours. Consider this my (binding) +1. >>>>>>> >>>>>>> Airflow 1.10.0 RC 3 is available at: >>>>>>> >>>>>>> https://dist.apache.org/repos/dist/dev/incubator/airflow/1.10.0rc3/ < >>>>>>> https://dist.apache.org/repos/dist/dev/incubator/airflow/1.10.0rc3/> >>>>>>> >>>>>>> apache-airflow-1.10.0rc3+incubating-source.tar.gz is a source release >>>>>>> that >>>>>>> comes with INSTALL instructions. >>>>>>> apache-airflow-1.10.0rc3+incubating-bin.tar.gz is the binary Python >>>>>>> "sdist" >>>>>>> release. >>>>>>> >>>>>>> Public keys are available at: >>>>>>> >>>>>>> https://dist.apache.org/repos/dist/release/incubator/airflow/ < >>>>>>> https://dist.apache.org/repos/dist/release/incubator/airflow/> >>>>>>> >>>>>>> The amount of JIRAs fixed is over 700. Please have a look at the >>>>>>> changelog. >>>>>>> Since RC2 the following has been fixed: >>>>>>> >>>>>>> * [AIRFLOW-2817] Force explicit choice on GPL dependency >>>>>>> * [AIRFLOW-2716] Replace async and await py3.7 keywords >>>>>>> * [AIRFLOW-2810] Fix typo in Xcom model timestamp >>>>>>> >>>>>>> Please note that the version number excludes the `rcX` string as well >>>>>>> as the "+incubating" string, so it's now simply 1.10.0. This will allow >>>>>>> us >>>>>>> to rename the artifact without modifying the artifact checksums when we >>>>>>> actually release. >>>>>>> >>>>>>> WARNING: Due to licensing requirements you will need to set >>>>>>> SLUGIFY_USES_TEXT_UNIDECODE=yes in your environment when >>>>>>> installing or upgrading. We will try to remove this requirement for the >>>>>>> next release. >>>>>>> >>>>>>> Cheers, >>>>>>> Bolke >>>> >>> >>
