Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Date: Add DateFormatters class that uses java.time #30612

Closed

Conversation

spinscale
Copy link
Contributor

A newly added class called DateFormatters now contains java.time based
builders for dates, which also intends to be fully backwards compatible,
when the name based date formatters are picked.

A duelling test class has been added that ensures the same dates when
parsing java or joda time formatted dates for the name based dates.

A first user of this class is the ingest module, which now uses
java.time under the hood instead of joda time.

Note, that java.time and joda time are not fully backwards compatible,
which also means that old formats will currently not work with this
setup.

spinscale added 4 commits May 15, 2018 14:20
A newly added class called DateFormatters now contains java.time based
builders for dates, which also intends to be fully backwards compatible,
when the name based date formatters are picked.

A duelling test class has been added that ensures the same dates when
parsing java or joda time formatted dates for the name based dates.

A first user of this class is the ingest module, which now uses
java.time under the hood instead of joda time.

Note, that java.time and joda time are not fully backwards compatible,
which also means that old formats will currently not work with this
setup.
@spinscale spinscale added review :Core/Infra/Core Core issues without another label labels May 15, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@spinscale spinscale force-pushed the 1805-add-java-time-date-formats branch from 4728a07 to 804c56f Compare May 16, 2018 09:29
@spinscale
Copy link
Contributor Author

@elasticmachine retest this please

@spinscale
Copy link
Contributor Author

this fails on CI due to a discrepancy between java8 and java 10... java 10 works as expected, but java 8 fails, this is a test that will not work on java8 but will on java10

    // java 10 can parse all of these formats just using "+HH:mm"
    // java 8 can not, furthermore even optional formatters seem to break
    // requires more investigation
    // possible candidates for fixes in the openjdk source
    // https://github.com/dmlloyd/openjdk/commit/c4a9c77a0314826b49a98a991035545f768a421a
    // https://github.com/dmlloyd/openjdk/commit/dd0368122c35c6a34f322a633bf98c91b50a3c30
    public void testBrokenWithJava8ButWorksWithJava10() {
        // works
        DateTimeFormatter f1 = new DateTimeFormatterBuilder().appendOffset("+HH:mm", "Z").toFormatter(Locale.ROOT);
        f1.parse("Z");
        f1.parse("-08");
        f1.parse("+08:00");

        // works
        DateTimeFormatter f2 = new DateTimeFormatterBuilder().appendOffset("+HHmm", "Z").toFormatter(Locale.ROOT);
        f2.parse("Z");
        f2.parse("-0800");

        // fails, only the first formatter is able top parse in java8
        DateTimeFormatter f3 = new DateTimeFormatterBuilder().appendOptional(f1).appendOptional(f2).toFormatter(Locale.ROOT);
        f3.parse("Z");
        f3.parse("-08");
        f3.parse("+08:00");
        f3.parse("-0800");
    }

spinscale and others added 17 commits July 5, 2018 09:14
Updates the build.gradle to take into account the OS differences for
Windows (in particular line separator and project naming)
…1804)

Job updates or changes to calendars or filters may
result into updating the job process if it has been
running. To preserve the order of updates, process
updates are queued through the UpdateJobProcessNotifier
which is only running on the master node. All actions
performing such updates must run on the master node.

However, the CRUD actions for calendars and filters
are not master node actions. They have been submitting
the updates to the UpdateJobProcessNotifier even though
it might have not been running (given the action was
run on a non-master node). When that happens, the update
never reaches the process.

This commit fixes this problem by ensuring the notifier
runs on all nodes and by ensuring the process update action
gets the resources again before updating the process
(instead of having those resources passed in the request).

This ensures that even if the order of the updates
gets messed up, the latest update will read the latest
state of those resource and the process will get back
in sync.

This leaves us with 2 types of updates:

  1. updates to the job config should happen on the master
  node. This is because we cannot refetch the entire job
  and update it. We need to know the parts that have been changed.

  2. updates to resources the job uses. Those can be handled
  on non-master nodes but they should be re-fetched by the
  update process action.

Closes elastic#31803
…tic#31800)

Job persistent tasks with stale allocation IDs used to always be
considered as OPENING jobs in the ML job node allocation decision.
However, FAILED jobs are not relocated to other nodes, which leads
to them blocking up the nodes they failed on after node restarts.
FAILED jobs should not restrict how many other jobs can open on a
node, regardless of whether they are stale or not.

Closes elastic#31794
…stic#31817)

It seems that java 11 tightened some validations with regard to
time formats. The random instance creator was setting an odd
time format to the data description which is invalid when run
with java 11. This commit changes it to a valid format.
…ic#31394)

Removes support for storing scripts without the usual json around the
script. So You can no longer do:
```
POST _scripts/<templatename>
{
    "query": {
        "match": {
            "title": "{{query_string}}"
        }
    }
}
```

and must instead do:
```
POST _scripts/<templatename>
{
    "script": {
        "lang": "mustache",
        "source": {
            "query": {
                "match": {
                    "title": "{{query_string}}"
                }
            }
        }
    }
}
```

This improves error reporting when you attempt to store a script but don't
quite get the syntax right. Before, there was a good chance that we'd
think of it as a "raw" template and just store it. Now we won't do that.
Nice.
At the end of every `ESRestTestCase` we clean the cluster which includes
deleting all of the templates. If xpack is installed it'll automatically
recreate a few templates every time they are removed. Which is slow.

This change stops the cleanup from removing the xpack templates. It cuts
the time to run the docs tests more than in half and it probably saves a
bit more time on other tests as well.
@rjernst
Copy link
Member

rjernst commented Jul 5, 2018

@spinscale I see this PR is against the java-time branch. Can this new DateFormatters class be added to master/6.x directly? I noticed there are a lot of unrelated diffs because the branch is out of date from master.

@spinscale
Copy link
Contributor Author

Superceded by #31856 (which is already in master)

@spinscale spinscale closed this Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants