-
Notifications
You must be signed in to change notification settings - Fork 105
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
[close #397] add git commit hash and version info while TiSession create #408
Conversation
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
logger.info("Release Version: " + properties.getProperty("git.build.version")); | ||
logger.info("Git Commit Hash: " + properties.getProperty("git.commit.id.full")); | ||
} catch (Exception e) { | ||
logger.error("Fail to read package info: " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to set it to info level since users don't need to perform any operation after seeing this log.
logger.info("Release Version: " + properties.getProperty("git.build.version")); | ||
logger.info("Git Commit Hash: " + properties.getProperty("git.commit.id.full")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's better to aggregate them into one log item to:
- reduce the log items and the total log size.
- reduce the log search (i.e.
grep
) burden
printVersionInfo(); | ||
} | ||
|
||
private void printVersionInfo() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about refactoring this function to:
private void getVersionInfo() String {
}
then log the info in TiSession
and add a UT for this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to write suck UT to assert the commit hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, I capture these logic into VersionInfo
, PTAL.
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
String commitHash = properties.getProperty("git.commit.id.full"); | ||
info = new VersionInfo(version, commitHash); | ||
} catch (Exception e) { | ||
logger.info("Fail to read package info: " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.info("Fail to read package info: " + e.getMessage()); | |
logger.error("Fail to read package info: " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conflicted with: #408 (comment). Personally speaking, I think the error log level could help us discover some building issues.
@@ -103,6 +120,22 @@ public TiSession(TiConfiguration conf) { | |||
warmUp(); | |||
this.circuitBreaker = new CircuitBreakerImpl(conf); | |||
logger.info("TiSession initialized in " + conf.getKvMode() + " mode"); | |||
logger.info("Welcome to TiKV Java Client {}", getVersionInfo()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would print a welcome message when every new TiSession
is created. Considering that an application can use multiple TiSession
s, is there a better place to print this message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
/run-all-tests |
Signed-off-by: iosmanthus myosmanthustree@gmail.com
What problem does this PR solve?
Add info logs while
Tisession
createIssue Number: close #397
Problem Description: TBD
What is changed and how it works?
Check List for Tests
This PR has been tested by at least one of the following methods:
Related changes