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

All Topic messages are processed by a single thread #7443

Closed
dsukhoroslov opened this issue Feb 1, 2016 · 15 comments

Comments

Projects
None yet
5 participants
@dsukhoroslov
Copy link
Contributor

commented Feb 1, 2016

I have a single HZ 3.5.4 server. There is a Topic. Both publisher and subscriber are within the server and they're singletons. The rate of messages published to the topic is quite high, it is > 1000 per sec. But all of them are consumed by only one HZ event thread, as I see in my logs. My EventService pool size is default = 5. So it is not clear why only one thread handles all the load. Look like you assign thread per subscriber? Is it possible to improve the situation somehow?

Thanks, Denis.

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2016

Hi @dsukhoroslov,

the same topic use the same thread. I agree this could be relaxed when ordering is not important. The workaround would be to either use multiple topics or offload message processing to another thread.

@jerrinot jerrinot added this to the Backlog milestone Feb 2, 2016

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2016

Hi @jerrinot,

I like this new label :). Ok, I'll think about it. Besides ordering do you have any other concens using the full EventService thread pool for message processing?

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2016

I am glad you like the label! ;-)

as far as I know ordering is the only reason for this.

This could be a good starting point See the last argument - name.hashCode())
Also please try to read the [documentation on this](http://docs.hazelcast.org/docs/3.6/manual/html-single/index.html#topic first). It's quite interesting. I didn't know about the globalOrderEnabled flag.

EDIT: If you are considering to implement this then feel free to discuss your ideas at https://gitter.im/hazelcast/hazelcast - I hang out there quite often.

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2016

@jerrinot, I looked at the code around TopicService and EventService. The improvement looks pretty clear:

add Registration.isOrdered() property, set it at construntion from nodeEngine.getHzInstance().getConfig().getTopicConfig(topic).isGlobalOrderingEnabled();

make two new classes which are not StrippedRunable, move most of LocalEventDispatcher/EventPacketPubliser fields to them (except orderKey);
inherit LocalEventDispatcher/EventPacketPubliser from these two classes, leave StrippedRunable implementation only;

in eventService.executeLocal construct LocalEventDisparcher or its non-StrinnedRunable parent depending on the reg.isOrdered() flag;

do the same for remote events with EventPacketPublisher/it's parent class;

the question is: do you want this behaviour be configurable or not? It is possible to add a new config property to topic (single-threaded true/false?) and use it as a flag instead of isOrdered(); with default value: true to leave current single-threaded behaviour as it is.

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2016

@jerrinot, if you agree with this implementation plan I'll do PR. I can add the new configuration element too. This will imply changes in HZ/HZ-Spring XSDs. Which HZ version should I fork, 3.7-SHAPSHOT?

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2016

Hi @dsukhoroslov,

looks good to me! Please make it's configurable with single-threaded default (=the current behaviour).

Base it on 3.7-SNAPSHOT I would prefer not to backport it to avoid possible regressions in patch releases.

Many thanks for your effort & idea!

@jerrinot jerrinot self-assigned this Feb 3, 2016

@noctarius

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2016

I guess ordered=true and multi-threaded is mutually exclusive, anyhow there might be people that still want single-threaded behavior but just don't care for the order, what do you think?

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2016

I agree. And yes, they'll get it with single-threaded=true, ordered = false config.

@vertex-github

This comment has been minimized.

Copy link

commented Feb 3, 2016

@noctarius Changing ordered processing to multi-threaded could break existing code (since everyone is expecting single thread behavior - or at least relying on that behavior even if they dont realize).

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2016

TopicConfig hashCode/equals methods implementation is confusing:

public int hashCode() {
    return (globalOrderingEnabled ? 1231 : 1237)
            + 31 * (name != null ? name.hashCode() : 0);
}

public boolean equals(Object obj) {
    if (this == obj) {
        return true;
    }
    if (!(obj instanceof TopicConfig)) {
        return false;
    }
    TopicConfig other = (TopicConfig) obj;
    return
            (this.name != null ? this.name.equals(other.name) : other.name == null)
                    && this.globalOrderingEnabled == other.globalOrderingEnabled;
}

Shouldn't it take into account the Topic name only? I don't beleive you want to have several topics with the same name but different ordering config. TopicService does not expect this, afaik.

@gurbuzali

This comment has been minimized.

Copy link
Member

commented Feb 3, 2016

Hi @dsukhoroslov you are right about your findings but since we hold the topicConfigs as name/TopicConfig map in Config, it is not likely that this causes any problem. it should be fixed nevertheless

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2016

Ok, I'll fix it as part of this PR.

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2016

Done. Please see PR #7472
I called the feature multi-threading-enabled, to be close to other topic options (statistics-enabled, global-ordering-enabled).

@dsukhoroslov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2016

The new PR for this issue: #7546

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Apr 20, 2016

added multi-threading processing for topic messages - test squash (#3)
* added multi-threading processing for topic messages; see hazelcast#7443

* improved counter

* improved config processing

* improved topic tests

* fixed checkstyle complains

* fixed checkstyle errors

* added multi-threading processing for topic messages; see hazelcast#7443
improved counter
improved config processing
improved topic tests
fixed checkstyle complains
fixed checkstyle errors

@jerrinot jerrinot modified the milestones: 3.7, Backlog Apr 20, 2016

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

fixed by #7546

@jerrinot jerrinot closed this Apr 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.