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

Data Loss bugfix / Additional Features #177

Closed
wants to merge 6 commits into from

Conversation

Daniel700
Copy link

@Daniel700 Daniel700 commented Jun 18, 2016

I guess that I've found the bugfix for the data loss problem:
The data loss happens if you send field values as strings which contain a backslash following with a java specific escape sequence. (See here: Escape Sequences)
The line protocol interprets then for example sometext\newtext with \n as a new line.
That's why I have added line 357 stringValue = stringValue.replace("\\", "/"); in Point.java as a quick fix. If you want a more sophisticated solution it's up to you. But it seems to work.

Moreover, you dont have to check the field (String) value for null in the addField() method (line 154 in Point.java) because you should also be able to add fields with null values. You just have to check that during the concatenateFields() method that you have at least one field with a value. There was also a bug in building the concatenated String if you pass null values (as a Number for example) that you had a comma at the end of the concatenated string where no comma should appear because no field has been added.

I have also added an additional Feature to addFields on basis of a condition. Since InfluxDB is a schemaless DB you don't need always to add every field to the Point class and transmit it to the Server. Please check the tests in the WriteTest.java class

@andrewdodd
Copy link
Contributor

Hi @Daniel700

Thanks for submitting a PR.

I am not sure I fully understand what you are trying to achieve with this set of changes. I'll try to cover off the different points below:

  1. The data loss that is discussed in Issue Points loss using batch processing #163 is related to the batch processor not retrying buffered writes if they fail. The changes in PR Data retention enhancement #108 are an attempt to fix this issue. I don't really see how these changes fix this issue?
  2. You have made changes to help with the escaping of field values. The change you made you probably make more sense if it was added to the initialisation of the FIELD_ESCAPER at Point.java line 31. However, I think that this is actually a concern for software that uses this library and not the library itself. For example, what if you want include a newline character into your string? With the changes in this PR it would be impossible.
  3. You have made it possible to add null to the value in fields. I'm not really sure why anyone would want to do this?
  4. You have added a number of methods to the public interface of the point builder with the form addField(String field, something, boolean activated);. I'm not sure I really understand these methods. I.e. why have 5 more methods just to push a boolean check inside the builder object?

As a result, I'm going to close this PR and recommend you to break the PR into smaller chunks.

Thanks,
Andrew

@andrewdodd andrewdodd closed this Jun 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants