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

Commands have Output fields that are nullable and mutable. #45

Closed
taer opened this issue Mar 1, 2015 · 8 comments
Closed

Commands have Output fields that are nullable and mutable. #45

taer opened this issue Mar 1, 2015 · 8 comments
Labels
type: enhancement A general enhancement

Comments

@taer
Copy link
Contributor

taer commented Mar 1, 2015

Commands can be created with null outputs. I think this could be cleaned up to require non-null Outputs. I think we should also remove the setOutput() call on command as well. Output is used in the RedisStateMachine, and changing output midstream or having a missing one can lead to issues.

From an initial look, only 2 commands have no output. The debugOOM and debugSegfault. Both these kill the server, so there is no output to report. I wasn't able to find any other places where a null output is used.

If this seems OK, I'd like to go down this path.

Also, is this the best way to discuss code proposals like this?

@mp911de
Copy link
Collaborator

mp911de commented Mar 1, 2015

Hi @taer,
this is a good starting point for improvement. I actually like the style of communication, face2face isn't always possible and there is no mailing list. So Github as communication platform is great.

You're right, currently only the debug commands have null outputs for not waiting on any response after firing the command. Guess, this can be improved. ClusterCommand does no magic when it comes to the outputs.

@mp911de mp911de added the type: enhancement A general enhancement label Mar 1, 2015
@taer
Copy link
Contributor Author

taer commented Mar 2, 2015

@mp911de What are your thoughts about adding jsr305?

<dependency>
    <groupId>com.google.code.findbugs</groupId>
    <artifactId>jsr305</artifactId>
    <version>3.0.0</version>
</dependency>

It would be a compile time dep, since I'd annotate methods with the Nonnull annotation, which means non-shaded downstream users would now need this.

Intellij helps analyze nulls if we can implement this.

@mp911de
Copy link
Collaborator

mp911de commented Mar 2, 2015

I'd rather go for Sonar in the first place. If you like, we could include it as <optional>true</optional>

@taer
Copy link
Contributor Author

taer commented Mar 2, 2015

I actually just noticed that guava depends on JSR305 already. And I just learned that annotations don't need to be present on the runtime classpath SO link

So an optional one won't affect users. Either way I'm planning to limit the annotation to a few initial methods.

I do like the idea of getting Sonar in place if it's an easy integration

@mp911de
Copy link
Collaborator

mp911de commented Mar 2, 2015

Go for it. Thanks for caring.

@mp911de
Copy link
Collaborator

mp911de commented Mar 4, 2015

lettuce is available on Nemo SonarQube http://nemo.sonarqube.org/dashboard/index/biz.paluch.redis:lettuce

@mp911de
Copy link
Collaborator

mp911de commented Mar 24, 2015

Added jsr305 lib directly in order to omit javadoc failures

mp911de added a commit that referenced this issue Mar 24, 2015
@mp911de
Copy link
Collaborator

mp911de commented Aug 25, 2015

Closed for now, no progress and no demand

@mp911de mp911de closed this as completed Aug 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants