[GP-4245] Debug logging for jira client #1290
Conversation
avarsava
force-pushed
the
GP-4245_logging
branch
from
6675539 to
933a4be
Compare
Makes sense! fixed |
|
Re-requesting review because I realized nothing had a proper toString so the debug logs would be less than useful and then everything spiraled out of control from there |
| protected abstract String getKType(); | ||
|
|
||
| protected abstract String getVType(); |
There was a problem hiding this comment.
getKeyType and getValueType would be more clear, but you're also basically defining these at the class level. Would it be more useful to just log the class name in toString rather than the type parameters? That would also be more concise and impossible to mess up when implementing a new subclass
There was a problem hiding this comment.
I designed it this way to minimize repeat code, if I moved toString to the class level then there would be ~20 lines of repeat code in each child class of KeyValueCache
There was a problem hiding this comment.
I meant that "KType" and "VType" are being defined at the class level right now - the return value is a static String for all implementations. This means the class name is just as useful, if not more so since 2 classes could have the same types. In KeyValueCache#toString you could just log getClass() (or one of its String properties) instead of needing getKType and getVType methods
There was a problem hiding this comment.
Didn't realize this would get the actual class name instead of the abstract class's name, thanks, fixed
| StringBuilder writeOut = new StringBuilder(); | ||
| writeOut.append("Action type filter of types: "); | ||
| for (String type : types) { | ||
| writeOut.append(type).append(", "); |
There was a problem hiding this comment.
Leaves an extra comma at the end, which could be confusing. Could use String.join instead, or delete from the StringBuilder after. Same pattern in a few other places too
| return writeOut.toString(); | ||
| } | ||
|
|
||
| protected abstract String getOperation(); |
There was a problem hiding this comment.
Would logging the class name here be just as useful too since these are all defined basically at the class level?
| @@ -69,4 +69,19 @@ public final void setEnd(Long end) { | |||
| public final void setStart(Long start) { | |||
| this.start = start; | |||
| } | |||
|
|
|||
| protected abstract String getName(); | |||
There was a problem hiding this comment.
Similarly, would the logging the class name be just as good?
avarsava
force-pushed
the
GP-4245_logging
branch
from
f0dc1c1 to
9d0908a
Compare
|
Looks good once compile errors are fixed (missed removing a few of the getKType/getVType/getOperation implementations) |
avarsava
force-pushed
the
GP-4245_logging
branch
from
9d0908a to
9b93bfa
Compare
JIRA Ticket: GP-4245