Skip to content
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

Return an error when Stream::Range fails and remove some debug server logs #1556

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

enjoy-binbin
Copy link
Member

@enjoy-binbin enjoy-binbin commented Jul 6, 2023

This PR has two parts.

The first one, internally, both XREAD and XRANGE call the Stream::Range
function. When Range fails, we should return an error.

The second one is to remove the debug logs. These logs were there in the
earliest days, i guess they are for debug, remove them now to avoid polluting
the server logs.

The blpop one is easy to reproduce:

Failed to execute redis command: blpop, err: Invalid argument: WRONGTYPE
Operation against a key holding the wrong kind of value

Checked the relevant LOG calls and fix a typo.

@PragmaTwice PragmaTwice requested a review from torwig July 10, 2023 03:35
@PragmaTwice
Copy link
Member

@xiaobiaozhao Why force push cf80ff7?

@xiaobiaozhao
Copy link
Contributor

@xiaobiaozhao Why force push cf80ff7?

rebase ?

@PragmaTwice
Copy link
Member

@xiaobiaozhao Why force push cf80ff7?

rebase ?

It is very weird. DO NOT force push to other contributor's branch without notification.

If you want to merge unstable to it, just click the "Update Branch" button, it will append a new commit.

@PragmaTwice
Copy link
Member

@xiaobiaozhao And I would like to state that I believe this is a very serious violation of the "unspoken agreement" among committers in git operations.

@git-hulk
Copy link
Member

Yes, you can append a new commit and SHOULD NOT rebase or force push to other contributors' PR.

@xiaobiaozhao
Copy link
Contributor

Yes, you can append a new commit and SHOULD NOT rebase or force push to other contributors' PR.

OK, got it

These logs were there in the earliest days, i guess
they are for debug, remove them now to avoid polluting
the server logs.

The blpop one is easy to reproduce:
```
Failed to execute redis command: blpop, err: Invalid argument: WRONGTYPE
Operation against a key holding the wrong kind of value
```

Checked the relevant LOG calls and fix a typo.
@enjoy-binbin enjoy-binbin changed the title Remove debug server logs Return an error when Stream::Range fails and remove some debug server logs Jul 17, 2023
@enjoy-binbin
Copy link
Member Author

i doing a force-push since it is a trivial diff.
For me, reset --hard unstable + cherry-pick + a new commit is clearer (and easier)

git-hulk
git-hulk previously approved these changes Jul 17, 2023
torwig
torwig previously approved these changes Jul 17, 2023
Copy link
Contributor

@torwig torwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@enjoy-binbin enjoy-binbin dismissed stale reviews from torwig and git-hulk via f493955 July 17, 2023 07:56
@git-hulk
Copy link
Member

Thanks all, merging...

@git-hulk git-hulk merged commit 82cc0a2 into apache:unstable Jul 18, 2023
@enjoy-binbin enjoy-binbin deleted the remove_debug_log branch July 18, 2023 04:34
git-hulk pushed a commit to git-hulk/kvrocks that referenced this pull request Jul 30, 2023
… logs (apache#1556)

This PR has two parts.

The first one, internally, both XREAD and XRANGE call the Stream::Range
function. When Range fails, we should return an error.

The second one is to remove the debug logs. These logs were there in the
earliest days, I guess they are for debugging, remove them now to avoid polluting
the server logs.

The blpop one is easy to reproduce:
```
Failed to execute redis command: blpop, err: Invalid argument: WRONGTYPE
Operation against a key holding the wrong kind of value
```
@git-hulk git-hulk mentioned this pull request Jul 31, 2023
git-hulk pushed a commit that referenced this pull request Aug 1, 2023
… logs (#1556)

This PR has two parts.

The first one, internally, both XREAD and XRANGE call the Stream::Range
function. When Range fails, we should return an error.

The second one is to remove the debug logs. These logs were there in the
earliest days, I guess they are for debugging, remove them now to avoid polluting
the server logs.

The blpop one is easy to reproduce:
```
Failed to execute redis command: blpop, err: Invalid argument: WRONGTYPE
Operation against a key holding the wrong kind of value
```
p1u3o pushed a commit to p1u3o/incubator-kvrocks that referenced this pull request Aug 1, 2023
… logs (apache#1556)

This PR has two parts.

The first one, internally, both XREAD and XRANGE call the Stream::Range
function. When Range fails, we should return an error.

The second one is to remove the debug logs. These logs were there in the
earliest days, I guess they are for debugging, remove them now to avoid polluting
the server logs.

The blpop one is easy to reproduce:
```
Failed to execute redis command: blpop, err: Invalid argument: WRONGTYPE
Operation against a key holding the wrong kind of value
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants