You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "mkevo (GitHub)" <gi...@apache.org> on 2019/09/24 12:48:03 UTC
[GitHub] [geode] mkevo opened pull request #4087: GEODE-7216 include
a timestamp in stack trace logs
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on pull request #4087:
GEODE-7216 include a timestamp in stack trace logs
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
Last request from my side, promise ✋: the `StackTracesPerMember` domain bean should have the full `Date` object, and the actual `Date` representation should be rendered in the "view" layer directly, so I'd move this part to the actual gfsh command as well.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on pull request #4087:
GEODE-7216 include a timestamp in stack trace logs
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
Why are you hardcoding the `Locale` to `UK` here?.
I think you should use `Locale.getDefault()` or [`DateTimeFormatter.ofPattern(String pattern)`](https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html#ofPattern-java.lang.String-) directly.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mkevo commented on pull request #4087: GEODE-7216
include a timestamp in stack trace logs
Posted by "mkevo (GitHub)" <gi...@apache.org>.
Do we need to use `try/catch` on formatter or override `toData/fromData` in `StackTracesPerMember`?
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aaronlindsey commented on pull request #4087:
GEODE-7216 include a timestamp in stack trace logs
Posted by "aaronlindsey (GitHub)" <gi...@apache.org>.
Would `StackTracesPerMember` ever be deserialized from an older version that didn't include the timestamp field? I'm just asking — I don't actually know. Maybe @jujoramos would know. If this could happen, then the timestamp field would be null and the `formatter.format(...)` line in `ExportStackTraceCommand` could throw a NPE.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on pull request #4087:
GEODE-7216 include a timestamp in stack trace logs
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
Small Nitpick: now that you're using `DateTimeFormatter` (immutable and thread safe) instead of `SimpleDateFormat`, you might want to create the formatter instance just once and re-use it instead of creating it every time the command is executed.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on pull request #4087:
GEODE-7216 include a timestamp in stack trace logs
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
Can we split these concepts (`memberId` and `timestamp`) in two different attributes?.
I don't see any good reason about why they should just be one single attributed created by a String concatenation (plus the `at` word). This logic is basically part of the "view" or representation and, as such, it should be handled directly in the gfsh command.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mkevo commented on pull request #4087: GEODE-7216
include a timestamp in stack trace logs
Posted by "mkevo (GitHub)" <gi...@apache.org>.
Thanks for comments! I added new test with null as the timestamp and modified existing to check if `headerMessage` has specific format as expected in output of this command.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mkevo commented on pull request #4087: GEODE-7216
include a timestamp in stack trace logs
Posted by "mkevo (GitHub)" <gi...@apache.org>.
Yes we can set it getDefault, I didn't see it. I just pick UK to be consistent with timeStamp in log file. I will change it. Thank for the comment!
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mkevo commented on issue #4087: GEODE-7216 include a
timestamp in stack trace logs
Posted by "mkevo (GitHub)" <gi...@apache.org>.
Thanks @aaronlindsey! Great article! I will change it.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on pull request #4087:
GEODE-7216 include a timestamp in stack trace logs
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
@aaronlindsey: good catch, I haven't thought about it but my guess is that it could happen, yes. I'm not a 100% sure, though.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on pull request #4087:
GEODE-7216 include a timestamp in stack trace logs
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
This will probably introduce flakiness in the tests as there's no guarantee that the `Date` created in the test will match (or be 1 second apart) from the one used in the actual command.
You'd need to use another approach to verify the changes; maybe check that the stack trace for each member **_contains_** a date and that the date follows the pattern you use within the command (`yyyy/MM/dd HH:mm:ss`)?.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mivanac commented on issue #4087: GEODE-7216 include
a timestamp in stack trace logs
Posted by "mivanac (GitHub)" <gi...@apache.org>.
Since these changes are approved, I will merge this PR.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mkevo commented on pull request #4087: GEODE-7216
include a timestamp in stack trace logs
Posted by "mkevo (GitHub)" <gi...@apache.org>.
It is possible to go without `Locale.getDefault()`, but we need to add `.withZone(ZoneId.systemDefault())`. To format an `Instant` a time-zone is required. Without a time-zone, the formatter does not know how to convert the instant to human date-time fields, and therefore throws an exception.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] aaronlindsey commented on pull request #4087:
GEODE-7216 include a timestamp in stack trace logs
Posted by "aaronlindsey (GitHub)" <gi...@apache.org>.
Thanks! It looks good.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on pull request #4087:
GEODE-7216 include a timestamp in stack trace logs
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
Can you change the method name to reflect what the test is doing and try to use _camelCase_ for the name instead of underscores?.
**_PD:_** I realise that other methods within the class use underscore, but I'd prefer us to use accepted formats when adding new code.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jujoramos commented on pull request #4087:
GEODE-7216 include a timestamp in stack trace logs
Posted by "jujoramos (GitHub)" <gi...@apache.org>.
This variable is redundant and not needed, you can just use `Instant.now()` when constructing the `StackTracesPerMember` instance.
[ Full content available at: https://github.com/apache/geode/pull/4087 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org