You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2021/07/24 04:48:48 UTC
[GitHub] [zeppelin] aib628 opened a new pull request #4186: [bugfix]Fast return when invalid ticket of no session case
aib628 opened a new pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186
### What is this PR for?
Fix NullPointerException of org.apache.zeppelin.socket.NotebookServer when invalid ticket message received
This exception may occur when server sessions is not contains received principal. In other words, it happend when no session case.
### What type of PR is it?
Bug Fix
### Todos
haven't
### What is the Jira issue?
[ZEPPELIN-5468] Fix NullPointerException of org.apache.zeppelin.socket.NotebookServer when invalid ticket message received
### How should this be tested?
Login visit page and then logout in other page, see log output
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] aib628 commented on pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
aib628 commented on pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#issuecomment-890303270
> @aib628 CI is failed, could you rebase and trigger it again ? And it would better to create PR against master branch
I had rebase and trigger it again, but CI still failed. How can i rerun and trigger it agagin when rebase branch no update?
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] aib628 commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
aib628 commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r686635143
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
This is an existing code fragment, and it means send logout messages to client when invalid ticket, i think we can keep it.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r685826902
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
Your screenshot shows a message that is logged at [line 466](https://github.com/apache/zeppelin/blob/eb0f8ab8c07c83f054a9de23aaa4edf9239e585c/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java#L466).
The debug log level is turned off by default. I think we should remove the first `if` and always log the line in the debug log level.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] asfgit closed pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r685128717
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
`receivedMessage.ticket` is empty or null at this time. I think that this message makes no sense.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] aib628 commented on pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
aib628 commented on pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#issuecomment-890512222
> @aib628 Just force push will trigger the CI, and BTW besides the NPE in log, is there any user experience effect of this issue ??
@zjffdu no, log only
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r686646534
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
As I mentioned earlier, debug output with an empty `receivedMessage.ticket` makes no sense, so I would remove the if and always output the invalid ticket in the debug output.
Your current code:
```java
} else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
} else if (!receivedMessage.op.equals(OP.PING)) {
conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info", "Your ticket is invalid possibly due to server restart. Please login again.")));
}
return;
}
```
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#issuecomment-898271801
If the CI goes through successfully, I will soon merge it into master.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r685826902
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
Your screenshot shows a message that is logged at [line 466](https://github.com/apache/zeppelin/blob/eb0f8ab8c07c83f054a9de23aaa4edf9239e585c/zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java#L466) with ERROR log level.
The debug log level is turned off by default. I think we should remove the first `if` and always log the line in the debug log level.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] aib628 commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
aib628 commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r686756136
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
good proposal, let me update it, thx.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r686646534
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
As I mentioned earlier, debug output with an empty `receivedMessage.ticket` makes no sense, so I would remove the if and always output the invalid ticket in the debug output.
Your current code:
```java
} else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
} else if (!receivedMessage.op.equals(OP.PING)) {
conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info", "Your ticket is invalid possibly due to server restart. Please login again.")));
}
return;
}
```
my proposal
```java
} else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
if (!receivedMessage.op.equals(OP.PING)) {
conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info", "Your ticket is invalid possibly due to server restart. Please login again.")));
}
return;
}
```
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] aib628 commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
aib628 commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r686447938
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
I don't quite understand what you mean, but i think a invalid ticket is no sence, we shoud log it in debug log level and fast return.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#issuecomment-886422188
@aib628 CI is failed, could you rebase and trigger it again ? And it would better to create PR against master branch
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] aib628 commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
aib628 commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r685807805
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
Yeah, i'm with you on that. But it appears, as shown in the screenshot, so we log it and fast return, other than execute downstram logic, and then resulting in NPE.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#issuecomment-890526987
@aib628 Recently CI seems unable recently, we may need to wait for a while until CI become stable.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zjffdu commented on pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#issuecomment-890331942
@aib628 Just force push will trigger the CI, and BTW besides the NPE in log, is there any user experience effect of this issue ??
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a change in pull request #4186: [ZEPPELIN-5468]Fast return when invalid ticket of no session case
Posted by GitBox <gi...@apache.org>.
Reamer commented on a change in pull request #4186:
URL: https://github.com/apache/zeppelin/pull/4186#discussion_r686590667
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
I mean this:
```
} else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
if (!receivedMessage.op.equals(OP.PING)) {
conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info", "Your ticket is invalid possibly due to server restart. Please login again.")));
}
return;
}
##########
File path: zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
##########
@@ -263,19 +263,17 @@ public void onMessage(NotebookSocket conn, String msg) {
}
TicketContainer.Entry ticketEntry = TicketContainer.instance.getTicketEntry(receivedMessage.principal);
- if (ticketEntry != null &&
- (!ticketEntry.getTicket().equals(receivedMessage.ticket))) {
+ if (ticketEntry == null || StringUtils.isEmpty(ticketEntry.getTicket())) {
+ LOG.debug("{} message: invalid ticket {}", receivedMessage.op, receivedMessage.ticket);
+ return;
+ } else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
/* not to pollute logs, log instead of exception */
if (StringUtils.isEmpty(receivedMessage.ticket)) {
- LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op,
- receivedMessage.ticket, ticketEntry.getTicket());
- } else {
- if (!receivedMessage.op.equals(OP.PING)) {
- conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info",
- "Your ticket is invalid possibly due to server restart. "
- + "Please login again.")));
- }
+ LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
Review comment:
I mean this:
```java
} else if (!ticketEntry.getTicket().equals(receivedMessage.ticket)) {
LOG.debug("{} message: invalid ticket {} != {}", receivedMessage.op, receivedMessage.ticket, ticketEntry.getTicket());
if (!receivedMessage.op.equals(OP.PING)) {
conn.send(serializeMessage(new Message(OP.SESSION_LOGOUT).put("info", "Your ticket is invalid possibly due to server restart. Please login again.")));
}
return;
}
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org