You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2021/07/29 20:48:50 UTC
[GitHub] [bookkeeper] mauricebarnum opened a new pull request #2758: state store: getStream: log stream info
mauricebarnum opened a new pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758
log the the tuple (namespace id, stream id, stream name) in RootStorageService getRange request.
### Motivation
Server request metrics are labeled with the stream id, extracted from the routing header. The stream name
(aka "table name") is not available but more useful. Rather than making a (cacheable) RPC request to fetch
the id -> name mapping in the metrics, logging the information allows one to find the name without requiring
admin access to the state store service.
> ---
> In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
> checks for pull requests. A pull request can only be merged when it passes precommit checks.
>
> ---
> Be sure to do all of the following to help us incorporate your contribution
> quickly and easily:
>
> If this PR is a BookKeeper Proposal (BP):
>
> - [ ] Make sure the PR title is formatted like:
> `<BP-#>: Description of bookkeeper proposal`
> `e.g. BP-1: 64 bits ledger is support`
> - [ ] Attach the master issue link in the description of this PR.
> - [ ] Attach the google doc link if the BP is written in Google Doc.
>
> Otherwise:
>
> - [ ] Make sure the PR title is formatted like:
> `<Issue #>: Description of pull request`
> `e.g. Issue 123: Description ...`
> - [ ] Make sure tests pass via `mvn clean apache-rat:check install spotbugs:check`.
> - [ ] Replace `<Issue #>` in the title with the actual Issue number.
>
> ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] sursingh commented on a change in pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
sursingh commented on a change in pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#discussion_r682938073
##########
File path: stream/storage/impl/src/main/java/org/apache/bookkeeper/stream/storage/impl/metadata/RootRangeStoreImpl.java
##########
@@ -692,7 +696,10 @@ StatusCode verifyStreamRequest(String nsName, String streamName) {
return FutureUtils.value(null);
} else {
try {
- return FutureUtils.value(StreamProperties.parseFrom(streamPropBytes));
+ final StreamProperties p = StreamProperties.parseFrom(streamPropBytes);
+ logger.info("namespace_id={} stream_id={} storage_container_id={} stream_name={} ",
+ nsId, p.getStreamId(), p.getStorageContainerId(), p.getStreamName());
+ return FutureUtils.value(p);
Review comment:
Now that we have the log in create/delete, we should remove the log from here. This will create a unnecessary noise in logs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hsaputra commented on pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
hsaputra commented on pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#issuecomment-900677737
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hsaputra commented on a change in pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
hsaputra commented on a change in pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#discussion_r683751734
##########
File path: stream/storage/impl/src/main/java/org/apache/bookkeeper/stream/storage/impl/metadata/RootRangeStoreImpl.java
##########
@@ -80,7 +80,7 @@
/**
* ns name key: [NS_NAME_TAG][ns_name].
*/
- static final byte[] getNamespaceNameKey(String nsName) {
+ static byte[] getNamespaceNameKey(String nsName) {
Review comment:
Removing the "final" here just refactor since not needed for static methods?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hsaputra commented on pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
hsaputra commented on pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#issuecomment-900677737
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] mauricebarnum commented on a change in pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
mauricebarnum commented on a change in pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#discussion_r683759445
##########
File path: stream/storage/impl/src/main/java/org/apache/bookkeeper/stream/storage/impl/metadata/RootRangeStoreImpl.java
##########
@@ -80,7 +80,7 @@
/**
* ns name key: [NS_NAME_TAG][ns_name].
*/
- static final byte[] getNamespaceNameKey(String nsName) {
+ static byte[] getNamespaceNameKey(String nsName) {
Review comment:
yes. cleans up some warnings in intellij. it's a separate commit because it's logically separate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] sursingh commented on a change in pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
sursingh commented on a change in pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#discussion_r680145286
##########
File path: stream/storage/impl/src/main/java/org/apache/bookkeeper/stream/storage/impl/metadata/RootRangeStoreImpl.java
##########
@@ -692,7 +696,10 @@ StatusCode verifyStreamRequest(String nsName, String streamName) {
return FutureUtils.value(null);
} else {
try {
- return FutureUtils.value(StreamProperties.parseFrom(streamPropBytes));
+ final StreamProperties p = StreamProperties.parseFrom(streamPropBytes);
+ logger.info("namespace_id={} stream_id={} storage_container_id={} stream_name={} ",
+ nsId, p.getStreamId(), p.getStorageContainerId(), p.getStreamName());
+ return FutureUtils.value(p);
Review comment:
We should do this only when we create/delete/modify streams. Also some context in the log will be useful. eg "Created Stream" etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] mauricebarnum commented on a change in pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
mauricebarnum commented on a change in pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#discussion_r682839021
##########
File path: stream/storage/impl/src/main/java/org/apache/bookkeeper/stream/storage/impl/metadata/RootRangeStoreImpl.java
##########
@@ -692,7 +696,10 @@ StatusCode verifyStreamRequest(String nsName, String streamName) {
return FutureUtils.value(null);
} else {
try {
- return FutureUtils.value(StreamProperties.parseFrom(streamPropBytes));
+ final StreamProperties p = StreamProperties.parseFrom(streamPropBytes);
+ logger.info("namespace_id={} stream_id={} storage_container_id={} stream_name={} ",
+ nsId, p.getStreamId(), p.getStorageContainerId(), p.getStreamName());
+ return FutureUtils.value(p);
Review comment:
moved logging to create and delete. add result of operation. since i'm only logging the (ns, id, name) tuple, i didn't add a log to modify for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hsaputra commented on pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
hsaputra commented on pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#issuecomment-900697368
Will merge this by tomorrow if no more question or concern. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] mauricebarnum commented on a change in pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
mauricebarnum commented on a change in pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#discussion_r682989373
##########
File path: stream/storage/impl/src/main/java/org/apache/bookkeeper/stream/storage/impl/metadata/RootRangeStoreImpl.java
##########
@@ -692,7 +696,10 @@ StatusCode verifyStreamRequest(String nsName, String streamName) {
return FutureUtils.value(null);
} else {
try {
- return FutureUtils.value(StreamProperties.parseFrom(streamPropBytes));
+ final StreamProperties p = StreamProperties.parseFrom(streamPropBytes);
+ logger.info("namespace_id={} stream_id={} storage_container_id={} stream_name={} ",
+ nsId, p.getStreamId(), p.getStorageContainerId(), p.getStreamName());
+ return FutureUtils.value(p);
Review comment:
oops. that was supposed to get removed. rebased.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] sursingh commented on a change in pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
sursingh commented on a change in pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#discussion_r682938073
##########
File path: stream/storage/impl/src/main/java/org/apache/bookkeeper/stream/storage/impl/metadata/RootRangeStoreImpl.java
##########
@@ -692,7 +696,10 @@ StatusCode verifyStreamRequest(String nsName, String streamName) {
return FutureUtils.value(null);
} else {
try {
- return FutureUtils.value(StreamProperties.parseFrom(streamPropBytes));
+ final StreamProperties p = StreamProperties.parseFrom(streamPropBytes);
+ logger.info("namespace_id={} stream_id={} storage_container_id={} stream_name={} ",
+ nsId, p.getStreamId(), p.getStorageContainerId(), p.getStreamName());
+ return FutureUtils.value(p);
Review comment:
Now that we have the log in create/delete, we should remove the log from here. This will create a unnecessary noise in logs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#discussion_r680141845
##########
File path: stream/storage/impl/src/main/java/org/apache/bookkeeper/stream/storage/impl/metadata/RootRangeStoreImpl.java
##########
@@ -64,6 +66,8 @@
public class RootRangeStoreImpl
implements RootRangeStore {
+ private static final Logger logger = LoggerFactory.getLogger(RootRangeStoreImpl.class);
Review comment:
You do not have to create this.
I think you can log just like that. Creation of logger is taken care of by @Slf4j
log.info("My log {}", myVar)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] mauricebarnum commented on a change in pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
mauricebarnum commented on a change in pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758#discussion_r682839021
##########
File path: stream/storage/impl/src/main/java/org/apache/bookkeeper/stream/storage/impl/metadata/RootRangeStoreImpl.java
##########
@@ -692,7 +696,10 @@ StatusCode verifyStreamRequest(String nsName, String streamName) {
return FutureUtils.value(null);
} else {
try {
- return FutureUtils.value(StreamProperties.parseFrom(streamPropBytes));
+ final StreamProperties p = StreamProperties.parseFrom(streamPropBytes);
+ logger.info("namespace_id={} stream_id={} storage_container_id={} stream_name={} ",
+ nsId, p.getStreamId(), p.getStorageContainerId(), p.getStreamName());
+ return FutureUtils.value(p);
Review comment:
moved logging to create and delete. add result of operation. since i'm only logging the (ns, id, name) tuple, i didn't add a log to modify for now.
##########
File path: stream/storage/impl/src/main/java/org/apache/bookkeeper/stream/storage/impl/metadata/RootRangeStoreImpl.java
##########
@@ -692,7 +696,10 @@ StatusCode verifyStreamRequest(String nsName, String streamName) {
return FutureUtils.value(null);
} else {
try {
- return FutureUtils.value(StreamProperties.parseFrom(streamPropBytes));
+ final StreamProperties p = StreamProperties.parseFrom(streamPropBytes);
+ logger.info("namespace_id={} stream_id={} storage_container_id={} stream_name={} ",
+ nsId, p.getStreamId(), p.getStorageContainerId(), p.getStreamName());
+ return FutureUtils.value(p);
Review comment:
oops. that was supposed to get removed. rebased.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hsaputra merged pull request #2758: state store: getStream: log stream info
Posted by GitBox <gi...@apache.org>.
hsaputra merged pull request #2758:
URL: https://github.com/apache/bookkeeper/pull/2758
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org