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

Use _doc if ES major version is 7 #9056

Merged
merged 26 commits into from Nov 29, 2018
Merged

Use _doc if ES major version is 7 #9056

merged 26 commits into from Nov 29, 2018

Conversation

@urso
Copy link
Collaborator

urso commented Nov 13, 2018

Update the Elasticsearch output and template generator to set the type
to _doc if Elasticsearch major version is 7.

@urso urso added the review label Nov 13, 2018
@urso urso requested review from tsg and ruflin Nov 13, 2018
Copy link
Collaborator

ruflin left a comment

This needs a changelog entry.

libbeat/outputs/elasticsearch/client.go Outdated Show resolved Hide resolved
libbeat/template/template.go Outdated Show resolved Hide resolved
libbeat/outputs/elasticsearch/client.go Show resolved Hide resolved
libbeat/kibana/client.go Show resolved Hide resolved
libbeat/common/version.go Show resolved Hide resolved
@urso urso force-pushed the urso:doc-type-for-7 branch from c7126d0 to c451beb Nov 15, 2018
Copy link
Collaborator

ruflin left a comment

Left a few minor comments. Code LGTM. Could you make hound happy?

CHANGELOG.asciidoc Show resolved Hide resolved
CHANGELOG.asciidoc Outdated Show resolved Hide resolved
filebeat/beater/filebeat.go Show resolved Hide resolved
filebeat/beater/filebeat.go Show resolved Hide resolved
filebeat/beater/filebeat.go Show resolved Hide resolved
@@ -69,6 +69,10 @@ func NewVersion(version string) (*Version, error) {
return &v, nil
}

func (v *Version) IsValid() bool {
return v.version != ""

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 19, 2018

Collaborator

Should this be called IsEmpty instead?

This comment has been minimized.

Copy link
@urso

urso Nov 20, 2018

Author Collaborator

Don't care about the name. stdlib seems to IsZero() bool for some types.

This comment has been minimized.

Copy link
@urso

urso Nov 20, 2018

Author Collaborator

Keeping IsValid cause of docstring:

IsValid returns true if the version object stores a successfully parsed version number.

The NewVersion constructor is no simple constructor, but actually a parser. Wonder why we parse ourselves. github.com/hashicorp/go-version is a pretty good and flexible versions package:

}

if majorVersion < 6 {
if esLoader.version.Major < 6 {

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 19, 2018

Collaborator

Not related to this PR but I wonder if we should remove support for this.

@jsoriano More related to your other PR with removing 5.x dashboards.

This comment has been minimized.

Copy link
@jsoriano

jsoriano Nov 19, 2018

Member

We can consider to keep this in libbeat in case some community beat still wants to try to keep support for 5.x.

If not, I can remove it in in #8927, along with the importViaES method.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 19, 2018

Collaborator

I would rather remove it to reduce complexity in the code. Can also be done in a follow up PR to keep things smaller.

var mappingName string
major := t.esVersion.Major
switch {
case major < 6:

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 19, 2018

Collaborator

I wonder if we should even allow to run Beats 7.0 against ES 5.x?

@monicasarbu @tsg

@urso

This comment has been minimized.

Copy link
Collaborator Author

urso commented Nov 20, 2018

@ruflin This PR will be for 7.0 and 6.7 the least. I'd say we should remove ES 5.x support in 7.0 in a followup PR (will add item to 7.0 bc meta issue).

@urso urso force-pushed the urso:doc-type-for-7 branch from c3a2891 to ae17426 Nov 22, 2018
@ruflin

This comment has been minimized.

Copy link
Collaborator

ruflin commented Nov 23, 2018

@urso Are the failing tests related to the flaky ones?

@urso

This comment has been minimized.

Copy link
Collaborator Author

urso commented Nov 23, 2018

Some tests are still failing. Some code seems in to sneak in a type named doc, so we end up with 2 types _doc and doc. Still trying to find the root cause.

@ph ph added the libbeat label Nov 23, 2018
urso added 17 commits Nov 13, 2018
Update the Elasticsearch output and template generator to set the type
to _doc if Elasticsearch major version is 7.
- Use common version instead of strings + parsing over and over again.
- Having the parsed version available earlier, we can now configure the
  default document type based on version ranges.
- Use `_doc` if version is >= 7.0.
@urso urso force-pushed the urso:doc-type-for-7 branch from 0fc79ee to 847c1c6 Nov 27, 2018
urso added 2 commits Nov 28, 2018
ups
@ruflin
ruflin approved these changes Nov 29, 2018
Copy link
Collaborator

ruflin left a comment

Only issue is changelog nit.

CHANGELOG.asciidoc Outdated Show resolved Hide resolved
marshaled, err = json.Marshal(content)
assert.NoError(t, err)
assert.Contains(t, string(marshaled), "beat.timezone")
cases := map[string]struct {

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 29, 2018

Collaborator

Much nicer 👍

@@ -105,8 +104,8 @@ func TestIngest(t *testing.T) {
}

client := getTestingElasticsearch(t)
if strings.HasPrefix(client.Connection.version, "2.") {
t.Skip("Skipping tests as pipeline not available in 2.x releases")
if client.Connection.version.Major < 5 {

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 29, 2018

Collaborator

Hopefully we can clean up this code one day as we should not test anymore against 2.x I think in the future.

@urso urso merged commit 41f87a4 into elastic:master Nov 29, 2018
4 checks passed
4 checks passed
CLA Commit author is a member of Elasticsearch
Details
Hound No violations found. Woof!
beats-ci Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
urso added a commit to urso/beats that referenced this pull request Dec 5, 2018
Update the Elasticsearch output and template generator to set the type
to _doc if Elasticsearch major version is 7.

Use common.Version throughout
- Use common version instead of strings + parsing over and over again.
- Having the parsed version available earlier, we can now configure the
  default document type based on version ranges.
- Use `_doc` if version is >= 7.0.

Introduce unit tests for version aware bulk encoding.

(cherry picked from commit 41f87a4)
@urso urso added the v6.6.0 label Dec 5, 2018
graphaelli added a commit to graphaelli/apm-server that referenced this pull request Dec 14, 2018
ycombinator added a commit that referenced this pull request Dec 28, 2018
…rsion (#9813)

Partial backport of #9056 — only the code changes related to changing the datatype of version fields/variables from `string` to `common.Version`.
ycombinator added a commit to ycombinator/beats that referenced this pull request Dec 28, 2018
…rsion (elastic#9813)

Partial backport of elastic#9056 — only the code changes related to changing the datatype of version fields/variables from `string` to `common.Version`.

(cherry picked from commit c23a0b4)
ycombinator added a commit that referenced this pull request Dec 28, 2018
…rsion (#9813) (#9815)

Partial backport of #9056 — only the code changes related to changing the datatype of version fields/variables from `string` to `common.Version`.

(cherry picked from commit c23a0b4)
@urso urso deleted the urso:doc-type-for-7 branch Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.