Skip to content

Conversation

willyborankin
Copy link
Contributor

@willyborankin willyborankin commented Nov 16, 2020

Add timestamp.resolution property to config timestamp extractor
There are two configuration parameter values:

  • milliseconds - extracts timestamp from the field or the key and puts it as is in milliseconds
  • seconds - extracts timestamp in seconds put in it in milliseconds

@HelenMel
Copy link
Contributor

@willyborankin left couple comments

Copy link
Collaborator

@ivanyu ivanyu left a comment

Choose a reason for hiding this comment

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

a couple of comments

@willyborankin willyborankin force-pushed the timestamp_extractor_configuration branch 2 times, most recently from eac7340 to 5cd3d5e Compare November 16, 2020 13:58
@willyborankin
Copy link
Contributor Author

@ivanyu and @HelenMel comments fixed

@willyborankin willyborankin force-pushed the timestamp_extractor_configuration branch from 5cd3d5e to 32c311c Compare November 16, 2020 14:41
@ivanyu
Copy link
Collaborator

ivanyu commented Nov 18, 2020

So, effectively we need this set of tests:

  • Struct + value is Long + resolution is seconds;
  • Map + value is Long + resolution is seconds;
  • Struct + value is Long + resolution is milliseconds;
  • Map + value is Long + resolution is milliseconds.

Some conversion according to the resolution should be done here.

Also:

  • Struct + value is Date + resolution is seconds and milliseconds;
  • Map + value is Date + resolution is seconds and milliseconds.

No conversion must be done here. Exactly the moment of time that is on the input should be on the output, despite the resolution setting.

There might be some way to use @ParametrizedTest to not copy too much code around.

@willyborankin willyborankin force-pushed the timestamp_extractor_configuration branch 2 times, most recently from c501dcc to ce94da1 Compare November 19, 2020 12:46
@willyborankin
Copy link
Contributor Author

@ivanyu all done

@willyborankin willyborankin force-pushed the timestamp_extractor_configuration branch from ce94da1 to c677cd9 Compare November 19, 2020 12:56
Copy link
Collaborator

@ivanyu ivanyu left a comment

Choose a reason for hiding this comment

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

A couple of small changes, but overall LGTM

add timestamp.resolution property to config timestamp extractor
There are two configuration parameter values:
- milliseconds - extracts timestamp from the field or the key and puts it as is in milliseconds
- seconds - extract seconds and put it in milliseconds
@willyborankin willyborankin force-pushed the timestamp_extractor_configuration branch from beffcdf to c3f1928 Compare November 19, 2020 20:57
@willyborankin
Copy link
Contributor Author

@ivanyu squashed

@ivanyu ivanyu merged commit 3a1746f into master Nov 20, 2020
@ivanyu ivanyu deleted the timestamp_extractor_configuration branch November 20, 2020 04:43
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.

4 participants