I believe some of the Geode community has already decided that we shouldn't
overuse default methods in interfaces. Dan and others were the ones who
decided this and thus I can't really explain to folks in PRs why it's bad
to overuse default methods. Could some of the folks with strong
understanding
The Geode/GemFire code base has never allowed declaring multiple fields or
vars on single line:
private MemberVM locator, server1, server2;
If spotless does NOT prevent this, then it's an oversight. Please do NOT do
the above.
If anyone feels strongly about changing the coding style to allow thi
Build Update for apache/geode-examples
-
Build: #448
Status: Passed
Duration: 23 mins and 45 secs
Commit: a7895d5 (develop)
Author: Robert Houghton
Message: Simplify fetch of Geode artifacts. Reset geode version re: GEODE-8016
Authored-by: Robert Houghton
Vi
As a general rule default implementations on an interface should only used when
absolutely necessary because all the implementations are out of your control.
For example, the collection interfaces in the JDK all gained new APIs but Java
doesn’t implement every instance of them so a default is ne
+1
> On May 8, 2020, at 10:09 AM, Kirk Lund wrote:
>
> The Geode/GemFire code base has never allowed declaring multiple fields or
> vars on single line:
>
> private MemberVM locator, server1, server2;
>
> If spotless does NOT prevent this, then it's an oversight. Please do NOT do
> the above.
Another way to think about this is:
1. First, default methods are not inherently bad. They are useful in many
situations and can be "overridden" on implementing classes, if necessary.
2. A default method should be provided when the operation is not strictly
required or if the implementation (proce
I'm reviewing lots of PRs which are
adding @SuppressWarnings("deprecation"), etc. I think this is a bad trend.
We should not be suppressing warnings in our code. That's hiding a problem
that we'll need to or want to deal with in the future. Also, if you add
several of these suppress annotations int
For context, here is an example of PR that added warnings as error
https://github.com/apache/geode/pull/4816. Here is the JIRA
https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7869
-michael
On Fri, May 8, 2020 at 11:45 AM Kirk Lund wrote:
> I'm reviewing lots of PRs which are
> addin
I submitted and PR a while ago that started making warnings errors on certain
modules. Through lazy consensus it was approved and merged. I would love to
apply it to more. To set a baseline, I tried to fix most of the deprecated and
other warnings as possible in the effected code. Many were so b
Another tip (for IJ IDEA users, probably same for Eclipse and other IDEs):
You can disable an inspection wher
On Fri, May 8, 2020 at 11:52 AM Michael Oleske wrote:
> For context, here is an example of PR that added warnings as error
> https://github.com/apache/geode/pull/4816. Here is the JIRA
Let's try this again :P.
+1 to Kirk's comments. Plus...
Another tip (for IJ IDEA users, probably same for Eclipse and other IDEs):
You can disable inspection for a warning that is otherwise benign (or not
correct) *rather than* unnecessarily annotating the code with
@SuppressWarnings.
However,
>
> You can disable inspection for a warning that is otherwise benign...
>
Is there a consensus on what constitutes a benign warning? I think the only
times I use @SuppressWarnings is in parameterized tests to suppress the
unused method warning for the parameters method, or for unchecked casts, as
> On May 8, 2020, at 12:41 PM, Donal Evans wrote:
>
> Is there a consensus on what constitutes a benign warning? I think the only
> times I use @SuppressWarnings is in parameterized tests to suppress the
> unused method warning for the parameters method, or for unchecked casts, as
> below:
>
@Donal -
Well, if you have code like...
public void someMethod(@Nullable Object value) {
Assert.notNull(value, "...");
value.invokeSomeMethod();
...
}
The compiler will often *warn* you that value might be null without a
proper null check. That is, not all IDEs recognize "valid"
I should clarify...
The use of ["rawtypes", "unchecked"] are quite common in test code and
"unused" is common in API/Framework (production) code
While *tests* make more liberal use of these checks ("unused" is
questionable (!), though), I think all of these checks should be used
judiciously in p
Default interface methods are especially appropriate when the new method can be
defined in terms of other existing methods in the interface. For examples,
when Collections added isEmpty(), it is basically just a shorthand for
length()==0 [but certain subclasses might still be able to provide a
Actually there is an alternative to using @SuppressWarnings for Mockito
mocks:
Region region = cast(mock(Region.class));
private static T cast(Object object) {
return (T) object;
}
Personally, I'd prefer to see warnings or workarounds like above than see
lots of @SuppressWarnings littered thr
> *appropriate when the new method can be defined in terms of other
existing methods in the interface*
This is what it means when a method employs the "template" design pattern.
Correction to my (earlier) example:
@FunctionalInterace
interface Sorter {
default Object[] sort(Object... array
I want to avoid having Java Interfaces in which every method (or even many
methods) has a default empty implementation. I think that's ok for callback
interfaces like CacheListener, but it's horrible for interfaces like Cache,
Region, InternalCache, InternalRegion. I don't think these non-callback
> On May 8, 2020, at 1:08 PM, Kirk Lund wrote:
>
> Actually there is an alternative to using @SuppressWarnings for Mockito
> mocks:
>
> Region region = cast(mock(Region.class));
>
> private static T cast(Object object) {
> return (T) object;
> }
The cast method will need to suppress unche
> On May 8, 2020, at 1:11 PM, Kirk Lund wrote:
>
> I want to avoid having Java Interfaces in which every method (or even many
> methods) has a default empty implementation. I think that's ok for callback
> interfaces like CacheListener, but it's horrible for interfaces like Cache,
> Region, Int
Agreed, but the following (inside tests) does not work in all cases, i.e.
Region region...
Particularly if "region" is passed to a method with a different type
signature.
I am trying to find/think of the situation I encounter from time to time,
even when I use the *wildcard* signature (i.e. Regi
How does everyone feel about situations involving raw types?
Such as public interface A is returning Region and not Region is that an
acceptable suppression?
Thanks,
Mark
> On May 8, 2020, at 1:20 PM, John Blum wrote:
>
> Agreed, but the following (inside tests) does not work in all cases, i.
It is slightly different, but here is a case I recently found.
List versionTags = versions.getVersionTags();
This method is internal though.
public List getVersionTags() {
return Collections.unmodifiableList(this.versionTags);
}
Thanks,
Mark
> On May 8, 2020, at 1:25 PM, Mark Hanson wrote:
Regarding...
*> public interface A is returning Region and not Region is that an
acceptable suppression?*
I think Geode API/Framework (e.g. PDX) (production) code should be very
explicit and *not* use *rawtypes*, or even *wildcard* types, for that
matter, as far as possible.
To be clear, I am sa
I've been using this class in geode-core mostly for tests:
package org.apache.geode.internal.cache.util;
import org.apache.geode.cache.execute.Execution;
@SuppressWarnings({"unchecked", "unused"})
public class UncheckedUtils {
public static T cast(Object object) {
return (T) object;
}
What's the recommendation for the legitimate usage of the deprecated
property? In my case, a gemFire property is deprecated, but "start locator"
command still has an option to turn on that property (the option is
deprecated as well, but we are still obligated to keep it in the code). In
this case,
> On May 8, 2020, at 1:42 PM, Kirk Lund wrote:
>
> I've been using this class in geode-core mostly for tests:
>
> package org.apache.geode.internal.cache.util;
>
> import org.apache.geode.cache.execute.Execution;
>
> @SuppressWarnings({"unchecked", "unused"})
> public class UncheckedUtils {
> On May 8, 2020, at 1:55 PM, Jinmei Liao wrote:
>
> What's the recommendation for the legitimate usage of the deprecated
> property? In my case, a gemFire property is deprecated, but "start locator"
> command still has an option to turn on that property (the option is
> deprecated as well, but
I'll file a ticket to improve this class.
On Fri, May 8, 2020 at 2:24 PM Jacob Barrett wrote:
>
>
> > On May 8, 2020, at 1:42 PM, Kirk Lund wrote:
> >
> > I've been using this class in geode-core mostly for tests:
> >
> > package org.apache.geode.internal.cache.util;
> >
> > import org.apache.g
> On May 8, 2020, at 2:30 PM, Kirk Lund wrote:
>
> I'll file a ticket to improve this class.
There is a project geode-unsafe, perhaps it belongs in there, if not already
there. Even geode-common works. That way it can be used in both test and
production code. It is useful for addressing sho
I’d like to bring GEODE-8068 to support/1.13. This commit reverts two prior
commits (GEODE-8033 and GEODE-8044). GEODE-8033 and GEODE-8044 introduced an
experimental feature that is useless in its current state and has already been
redesigned, so there is no reason for it to go out with 1.13.
R
+1
Redis work is still marked @Experimental, but since this was reverted on
develop just after the branch cut, it makes sense to revert from support/1.13
as well.
> On May 8, 2020, at 2:40 PM, Patrick Johnson wrote:
>
> I’d like to bring GEODE-8068 to support/1.13. This commit reverts two pri
+1
On Fri, May 8, 2020 at 2:52 PM Owen Nichols wrote:
> +1
>
> Redis work is still marked @Experimental, but since this was reverted on
> develop just after the branch cut, it makes sense to revert from
> support/1.13 as well.
>
> > On May 8, 2020, at 2:40 PM, Patrick Johnson wrote:
> >
> > I’d
Hello all,
This is a call for help.
We have recently enabled an API check job on pull requests on Concourse.
You have likely seen it, since it is failing while comparing changes on
develop (not yet on support/1.13) to our latest release, v1.12.0.
I have configuration changes to the tool itself t
It's not the test code, it's the "start locator" command itself.
On Fri, May 8, 2020 at 2:27 PM Jacob Barrett wrote:
>
> > On May 8, 2020, at 1:55 PM, Jinmei Liao wrote:
> >
> > What's the recommendation for the legitimate usage of the deprecated
> > property? In my case, a gemFire property is
This change was just discussed yesterday on the dev list. Jake said:
> I have some concerns with using Properties in public APIs. The use of
> Properties is not strongly typed. I can’ tell from one property to the next
> what the type is. I can’t get compile time errors is the type is wrong. I
Hey Ya’ll,
We have a new API going into 1.13 that has an inconsistency I want to address
before we are stuck with it. The class SniSocketFactory should be renamed
SniProxySocketFactory. The class in question produces objects of type
SniProxySocket. An "SNI socket" isn’t a thing but an SNI proxy
> On May 8, 2020, at 3:41 PM, Jinmei Liao wrote:
>
> It's not the test code, it's the "start locator" command itself.
>
Same rules apply I think. If the method is deprecated don’t use it. If the
method using it is intentionally using the deprecated behavior it too should be
deprecated, thus
+1
On Fri, May 8, 2020, 18:26 Jacob Barrett wrote:
> Hey Ya’ll,
>
> We have a new API going into 1.13 that has an inconsistency I want to
> address before we are stuck with it. The class SniSocketFactory should be
> renamed SniProxySocketFactory. The class in question produces objects of
> type
+1
> On May 8, 2020, at 7:57 PM, Robert Houghton wrote:
>
> +1
>
> On Fri, May 8, 2020, 18:26 Jacob Barrett wrote:
>
>> Hey Ya’ll,
>>
>> We have a new API going into 1.13 that has an inconsistency I want to
>> address before we are stuck with it. The class SniSocketFactory should be
>> renam
+1
On Fri, May 8, 2020 at 7:59 PM Owen Nichols wrote:
> +1
>
> > On May 8, 2020, at 7:57 PM, Robert Houghton
> wrote:
> >
> > +1
> >
> > On Fri, May 8, 2020, 18:26 Jacob Barrett wrote:
> >
> >> Hey Ya’ll,
> >>
> >> We have a new API going into 1.13 that has an inconsistency I want to
> >> addr
42 matches
Mail list logo
