[GitHub] [incubator-dubbo] kezhenxu94 opened issue #3106: Should org.apache.dubbo.registry.support.AbstractRegistry#getRegistered returns unmodifiable sets?

["kezhenxu94 (GitHub)" <gi...@apache.org>]

I notice that the method `org.apache.dubbo.registry.support.AbstractRegistry#getRegistered` just returns the internal reference of `registered`, and everyone can modify this `Set` outside the `Registry`, is this a good practice?

An example is that in `org.apache.dubbo.registry.support.AbstractRegistryTest#testRegister`, there is something like:

```java
//test multiple urls
abstractRegistry.getRegistered().clear();
```

we clear the registered urls outside the `Registry`, maybe it's not a big deal because we just do some logs and remove the url in `unregister` method:

```java
    @Override
    public void unregister(URL url) {
        if (url == null) {
            throw new IllegalArgumentException("unregister url == null");
        }
        if (logger.isInfoEnabled()) {
            logger.info("Unregister: " + url);
        }
        registered.remove(url);
    }
```

but still, it'll lose some logs and if we add more logics in the `unregister` method in the future, it could be dangerous.

I suggest returning a unmodifiable set in the `getRegistered` method and unregister all urls one by one or providing a method such as `unregisterAll`.

What do you think?

[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3106 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3106: Should org.apache.dubbo.registry.support.AbstractRegistry#getRegistered returns unmodifiable sets?

["khanimteyaz (GitHub)" <gi...@apache.org>]

@kezhenxu94 , what you mentioned I am with it. I think we should keep the responsibility of operation on objects e.g. in this case clearing the registry to the owner of registries object AbstractRegistry.java and when ever it returns any mutable member variable it should return copy of it. But having said that we also needs to consider memory footprint/performance of creating clone copy of returned object. I beleve it should not change memory footprint or performance drastically but better to have some data around it.

[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3106 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] beiwei30 closed issue #3106: Should org.apache.dubbo.registry.support.AbstractRegistry#getRegistered returns unmodifiable sets?

["beiwei30 (GitHub)" <gi...@apache.org>]

[ issue closed by beiwei30 ]

[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3106 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] khanimteyaz commented on issue #3106: Should org.apache.dubbo.registry.support.AbstractRegistry#getRegistered returns unmodifiable sets?

["khanimteyaz (GitHub)" <gi...@apache.org>]

> > I beleve it should not change memory footprint or performance drastically but better to have some data around it.
> 
> @khanimteyaz That's my opinion too. What I suggest is to wrap the member variable such as using `Collections.unmodifiableSet`, and this may break something, so it needs investigation and discussion

@kezhenxu94 agree.

[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3106 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] kezhenxu94 commented on issue #3106: Should org.apache.dubbo.registry.support.AbstractRegistry#getRegistered returns unmodifiable sets?

["kezhenxu94 (GitHub)" <gi...@apache.org>]

> I beleve it should not change memory footprint or performance drastically but better to have some data around it.

@khanimteyaz That's my opinion too. What I suggest is to wrap the member variable such as using `Collections.unmodifiableSet`, and this may break something, so it needs investigation and discussion

[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3106 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] beiwei30 commented on issue #3106: Should org.apache.dubbo.registry.support.AbstractRegistry#getRegistered returns unmodifiable sets?

["beiwei30 (GitHub)" <gi...@apache.org>]

then, can someone nicely work on it?

[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3106 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] kezhenxu94 commented on issue #3106: Should org.apache.dubbo.registry.support.AbstractRegistry#getRegistered returns unmodifiable sets?

["kezhenxu94 (GitHub)" <gi...@apache.org>]

@beiwei30  I'll work on it

[ Full content available at: https://github.com/apache/incubator-dubbo/issues/3106 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org