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

Using default:null for _timestamp field creates a index loss on restart #9233

Closed
wants to merge 1 commit into from

Conversation

dadoonet
Copy link
Member

Note: this issue won't appear anymore in elasticsearch 1.5 and above because #9104 will disable support for "default":null. That being said, I think it would be safer to port this patch to 1.x and master branches.

Step to reproduce:

  • Create new index and type.
DELETE new_index
PUT new_index
 {
    "mappings": {
        "power": {
            "_timestamp" : {
                "enabled" : true,
                "default": null
            }
        }
    }
}
  • Add a document
PUT new_index/power/1
{
    "foo": "bar"
}
  • Restart cluster ... and index is missing...
GET new_index

Gives IndexMissingException

Closes #9223.

… on restart

Step to reproduce:

* Create new index and type.

```
DELETE new_index
PUT new_index
 {
    "mappings": {
    	"power": {
	        "_timestamp" : {
	            "enabled" : true,
	            "default": null
	        }
	    }
    }
}
```

* Add a document

```
PUT new_index/power/1
{
    "foo": "bar"
}
```

* Restart cluster ... and **index is missing**...

```
GET new_index
```

Gives IndexMissingException

Closes elastic#9223.
@dadoonet dadoonet added review v1.4.3 v1.5.0 v2.0.0-beta1 :Search/Mapping Index mappings, including merging and defining field types >bug critical and removed critical labels Jan 10, 2015
@dadoonet dadoonet self-assigned this Jan 10, 2015
@@ -354,7 +354,7 @@ private void initMappers(Map<String, Object> withoutType) {
path = fieldNode.toString();
} else if (fieldName.equals("format")) {
format = fieldNode.toString();
} else if (fieldName.equals("default")) {
} else if (fieldName.equals("default") && fieldNode != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the same NPE possible above in all the other conditions? Should we just continue the loop before these checks?

if (fieldNode == null) continue;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid point. Though I think null values might be rejected at mapping creation time for other values.
But as it's a safer option, I'll modify the PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original reporter here who discovered this issue after using _timestamp with default=null.

For "default" field in particular, null actually means something (which is why i used it) - to ignore the normal _timestamp default of now if _timestamp is not provided, thereby making _timestamp (or the field providing it via path property) a required field. seems like "default" property needs separate handling from other fields that might inadvertently have been passed a null - as in this case passing a null has a meaning (that being: clear any default behavior that the _timestamp should have).

From _timestamp doc, section on Default.

By default, the default value is now which means the date the document was processed by the indexing chain.

You can disable that default value by setting default to null. It means that timestamp is mandatory:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw -in the case where you inject a record without provided path property being there, and have set default=null, you'll properly get this error, as expected per the documentation quoted above.

{
    "error": "TimestampParsingException[failed to parse timestamp [timestamp is required by mapping]]",
    "status": 500
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic subscribing to what is described in the docs (ignoring null assigned to any other properties) would be more like this... making defaultTimestamp null if so, else the string provided.

else if (fieldName.equals("default")) {
      defaultTimestamp = (fieldNode == null ? null : fieldNode.toString());
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 1.5 we will change this logic: see #9104

@s1monw
Copy link
Contributor

s1monw commented Jan 11, 2015

@jpountz please review this before it gets merged

@dadoonet
Copy link
Member Author

I think we should also modify the log level if we can not open an index. For now this error could only be seen when in DEBUG level.

@jpountz
Copy link
Contributor

jpountz commented Jan 12, 2015

LGTM

@dadoonet
Copy link
Member Author

Closed with e654a2c be1610b and aef3bc2

@dadoonet dadoonet closed this Jan 13, 2015
@meash-nrel
Copy link

regardless of 1.5 changing the nature of the feature, for 1.4.x, I still say this fix is going to break the intended default=null behavior that the docs state, as the "default" property will continue to default to "now" even when user sets default=null to disable that behavior (per doc snippit I quoted above).

@dadoonet dadoonet deleted the pr/9223-timestamp-npe-startup branch January 26, 2015 21:41
@clintongormley clintongormley changed the title [Mapper] Using default=null for _timestamp field creates a index loss on restart Mapping: Using default=null for _timestamp field creates a index loss on restart Feb 10, 2015
@clintongormley clintongormley changed the title Mapping: Using default=null for _timestamp field creates a index loss on restart Using default:null for _timestamp field creates a index loss on restart Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Mapping Index mappings, including merging and defining field types v1.4.3 v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants