Showing
4 changed files
with
291 additions
and
4 deletions
| ... | @@ -4,9 +4,11 @@ import com.google.common.base.Function; | ... | @@ -4,9 +4,11 @@ import com.google.common.base.Function; |
| 4 | import com.google.common.base.Predicate; | 4 | import com.google.common.base.Predicate; |
| 5 | import com.google.common.collect.FluentIterable; | 5 | import com.google.common.collect.FluentIterable; |
| 6 | import com.google.common.collect.HashMultimap; | 6 | import com.google.common.collect.HashMultimap; |
| 7 | +import com.google.common.collect.ImmutableList; | ||
| 7 | import com.google.common.collect.Maps; | 8 | import com.google.common.collect.Maps; |
| 8 | import com.google.common.collect.SetMultimap; | 9 | import com.google.common.collect.SetMultimap; |
| 9 | 10 | ||
| 11 | +import org.apache.commons.lang3.RandomUtils; | ||
| 10 | import org.apache.commons.lang3.concurrent.ConcurrentUtils; | 12 | import org.apache.commons.lang3.concurrent.ConcurrentUtils; |
| 11 | import org.apache.felix.scr.annotations.Activate; | 13 | import org.apache.felix.scr.annotations.Activate; |
| 12 | import org.apache.felix.scr.annotations.Component; | 14 | import org.apache.felix.scr.annotations.Component; |
| ... | @@ -15,6 +17,8 @@ import org.apache.felix.scr.annotations.Reference; | ... | @@ -15,6 +17,8 @@ import org.apache.felix.scr.annotations.Reference; |
| 15 | import org.apache.felix.scr.annotations.ReferenceCardinality; | 17 | import org.apache.felix.scr.annotations.ReferenceCardinality; |
| 16 | import org.apache.felix.scr.annotations.Service; | 18 | import org.apache.felix.scr.annotations.Service; |
| 17 | import org.onlab.onos.cluster.ClusterService; | 19 | import org.onlab.onos.cluster.ClusterService; |
| 20 | +import org.onlab.onos.cluster.ControllerNode; | ||
| 21 | +import org.onlab.onos.cluster.NodeId; | ||
| 18 | import org.onlab.onos.net.AnnotationsUtil; | 22 | import org.onlab.onos.net.AnnotationsUtil; |
| 19 | import org.onlab.onos.net.ConnectPoint; | 23 | import org.onlab.onos.net.ConnectPoint; |
| 20 | import org.onlab.onos.net.DefaultAnnotations; | 24 | import org.onlab.onos.net.DefaultAnnotations; |
| ... | @@ -47,18 +51,24 @@ import org.slf4j.Logger; | ... | @@ -47,18 +51,24 @@ import org.slf4j.Logger; |
| 47 | 51 | ||
| 48 | import java.io.IOException; | 52 | import java.io.IOException; |
| 49 | import java.util.Collections; | 53 | import java.util.Collections; |
| 54 | +import java.util.HashMap; | ||
| 50 | import java.util.HashSet; | 55 | import java.util.HashSet; |
| 51 | import java.util.Map; | 56 | import java.util.Map; |
| 52 | import java.util.Set; | 57 | import java.util.Set; |
| 53 | import java.util.Map.Entry; | 58 | import java.util.Map.Entry; |
| 54 | import java.util.concurrent.ConcurrentHashMap; | 59 | import java.util.concurrent.ConcurrentHashMap; |
| 55 | import java.util.concurrent.ConcurrentMap; | 60 | import java.util.concurrent.ConcurrentMap; |
| 61 | +import java.util.concurrent.ScheduledExecutorService; | ||
| 62 | +import java.util.concurrent.TimeUnit; | ||
| 56 | 63 | ||
| 64 | +import static java.util.concurrent.Executors.newSingleThreadScheduledExecutor; | ||
| 65 | +import static org.onlab.onos.cluster.ControllerNodeToNodeId.toNodeId; | ||
| 57 | import static org.onlab.onos.net.DefaultAnnotations.union; | 66 | import static org.onlab.onos.net.DefaultAnnotations.union; |
| 58 | import static org.onlab.onos.net.DefaultAnnotations.merge; | 67 | import static org.onlab.onos.net.DefaultAnnotations.merge; |
| 59 | import static org.onlab.onos.net.Link.Type.DIRECT; | 68 | import static org.onlab.onos.net.Link.Type.DIRECT; |
| 60 | import static org.onlab.onos.net.Link.Type.INDIRECT; | 69 | import static org.onlab.onos.net.Link.Type.INDIRECT; |
| 61 | import static org.onlab.onos.net.link.LinkEvent.Type.*; | 70 | import static org.onlab.onos.net.link.LinkEvent.Type.*; |
| 71 | +import static org.onlab.util.Tools.namedThreads; | ||
| 62 | import static org.slf4j.LoggerFactory.getLogger; | 72 | import static org.slf4j.LoggerFactory.getLogger; |
| 63 | import static com.google.common.collect.Multimaps.synchronizedSetMultimap; | 73 | import static com.google.common.collect.Multimaps.synchronizedSetMultimap; |
| 64 | import static com.google.common.base.Predicates.notNull; | 74 | import static com.google.common.base.Predicates.notNull; |
| ... | @@ -110,13 +120,30 @@ public class GossipLinkStore | ... | @@ -110,13 +120,30 @@ public class GossipLinkStore |
| 110 | } | 120 | } |
| 111 | }; | 121 | }; |
| 112 | 122 | ||
| 123 | + private ScheduledExecutorService executor; | ||
| 124 | + | ||
| 113 | @Activate | 125 | @Activate |
| 114 | public void activate() { | 126 | public void activate() { |
| 115 | 127 | ||
| 116 | clusterCommunicator.addSubscriber( | 128 | clusterCommunicator.addSubscriber( |
| 117 | - GossipLinkStoreMessageSubjects.LINK_UPDATE, new InternalLinkEventListener()); | 129 | + GossipLinkStoreMessageSubjects.LINK_UPDATE, |
| 130 | + new InternalLinkEventListener()); | ||
| 131 | + clusterCommunicator.addSubscriber( | ||
| 132 | + GossipLinkStoreMessageSubjects.LINK_REMOVED, | ||
| 133 | + new InternalLinkRemovedEventListener()); | ||
| 118 | clusterCommunicator.addSubscriber( | 134 | clusterCommunicator.addSubscriber( |
| 119 | - GossipLinkStoreMessageSubjects.LINK_REMOVED, new InternalLinkRemovedEventListener()); | 135 | + GossipLinkStoreMessageSubjects.LINK_ANTI_ENTROPY_ADVERTISEMENT, |
| 136 | + new InternalLinkAntiEntropyAdvertisementListener()); | ||
| 137 | + | ||
| 138 | + executor = | ||
| 139 | + newSingleThreadScheduledExecutor(namedThreads("link-anti-entropy-%d")); | ||
| 140 | + | ||
| 141 | + // TODO: Make these configurable | ||
| 142 | + long initialDelaySec = 5; | ||
| 143 | + long periodSec = 5; | ||
| 144 | + // start anti-entropy thread | ||
| 145 | + executor.scheduleAtFixedRate(new SendAdvertisementTask(), | ||
| 146 | + initialDelaySec, periodSec, TimeUnit.SECONDS); | ||
| 120 | 147 | ||
| 121 | log.info("Started"); | 148 | log.info("Started"); |
| 122 | } | 149 | } |
| ... | @@ -408,6 +435,10 @@ public class GossipLinkStore | ... | @@ -408,6 +435,10 @@ public class GossipLinkStore |
| 408 | NewConcurrentHashMap.<ProviderId, Timestamped<LinkDescription>>ifNeeded()); | 435 | NewConcurrentHashMap.<ProviderId, Timestamped<LinkDescription>>ifNeeded()); |
| 409 | } | 436 | } |
| 410 | 437 | ||
| 438 | + private Timestamped<LinkDescription> getLinkDescription(LinkKey key, ProviderId providerId) { | ||
| 439 | + return getLinkDescriptions(key).get(providerId); | ||
| 440 | + } | ||
| 441 | + | ||
| 411 | private final Function<LinkKey, Link> lookupLink = new LookupLink(); | 442 | private final Function<LinkKey, Link> lookupLink = new LookupLink(); |
| 412 | private Function<LinkKey, Link> lookupLink() { | 443 | private Function<LinkKey, Link> lookupLink() { |
| 413 | return lookupLink; | 444 | return lookupLink; |
| ... | @@ -448,6 +479,19 @@ public class GossipLinkStore | ... | @@ -448,6 +479,19 @@ public class GossipLinkStore |
| 448 | clusterCommunicator.broadcast(message); | 479 | clusterCommunicator.broadcast(message); |
| 449 | } | 480 | } |
| 450 | 481 | ||
| 482 | + // TODO: should we be throwing exception? | ||
| 483 | + private void unicastMessage(NodeId recipient, MessageSubject subject, Object event) { | ||
| 484 | + try { | ||
| 485 | + ClusterMessage message = new ClusterMessage( | ||
| 486 | + clusterService.getLocalNode().id(), | ||
| 487 | + subject, | ||
| 488 | + SERIALIZER.encode(event)); | ||
| 489 | + clusterCommunicator.unicast(message, recipient); | ||
| 490 | + } catch (IOException e) { | ||
| 491 | + log.error("Failed to send a {} message to {}", subject.value(), recipient); | ||
| 492 | + } | ||
| 493 | + } | ||
| 494 | + | ||
| 451 | private void notifyPeers(InternalLinkEvent event) throws IOException { | 495 | private void notifyPeers(InternalLinkEvent event) throws IOException { |
| 452 | broadcastMessage(GossipLinkStoreMessageSubjects.LINK_UPDATE, event); | 496 | broadcastMessage(GossipLinkStoreMessageSubjects.LINK_UPDATE, event); |
| 453 | } | 497 | } |
| ... | @@ -456,6 +500,125 @@ public class GossipLinkStore | ... | @@ -456,6 +500,125 @@ public class GossipLinkStore |
| 456 | broadcastMessage(GossipLinkStoreMessageSubjects.LINK_REMOVED, event); | 500 | broadcastMessage(GossipLinkStoreMessageSubjects.LINK_REMOVED, event); |
| 457 | } | 501 | } |
| 458 | 502 | ||
| 503 | + private void notifyPeer(NodeId peer, InternalLinkEvent event) { | ||
| 504 | + unicastMessage(peer, GossipLinkStoreMessageSubjects.LINK_UPDATE, event); | ||
| 505 | + } | ||
| 506 | + | ||
| 507 | + private void notifyPeer(NodeId peer, InternalLinkRemovedEvent event) { | ||
| 508 | + unicastMessage(peer, GossipLinkStoreMessageSubjects.LINK_REMOVED, event); | ||
| 509 | + } | ||
| 510 | + | ||
| 511 | + private final class SendAdvertisementTask implements Runnable { | ||
| 512 | + | ||
| 513 | + @Override | ||
| 514 | + public void run() { | ||
| 515 | + if (Thread.currentThread().isInterrupted()) { | ||
| 516 | + log.info("Interrupted, quitting"); | ||
| 517 | + return; | ||
| 518 | + } | ||
| 519 | + | ||
| 520 | + try { | ||
| 521 | + final NodeId self = clusterService.getLocalNode().id(); | ||
| 522 | + Set<ControllerNode> nodes = clusterService.getNodes(); | ||
| 523 | + | ||
| 524 | + ImmutableList<NodeId> nodeIds = FluentIterable.from(nodes) | ||
| 525 | + .transform(toNodeId()) | ||
| 526 | + .toList(); | ||
| 527 | + | ||
| 528 | + if (nodeIds.size() == 1 && nodeIds.get(0).equals(self)) { | ||
| 529 | + log.info("No other peers in the cluster."); | ||
| 530 | + return; | ||
| 531 | + } | ||
| 532 | + | ||
| 533 | + NodeId peer; | ||
| 534 | + do { | ||
| 535 | + int idx = RandomUtils.nextInt(0, nodeIds.size()); | ||
| 536 | + peer = nodeIds.get(idx); | ||
| 537 | + } while (peer.equals(self)); | ||
| 538 | + | ||
| 539 | + LinkAntiEntropyAdvertisement ad = createAdvertisement(); | ||
| 540 | + | ||
| 541 | + if (Thread.currentThread().isInterrupted()) { | ||
| 542 | + log.info("Interrupted, quitting"); | ||
| 543 | + return; | ||
| 544 | + } | ||
| 545 | + | ||
| 546 | + try { | ||
| 547 | + unicastMessage(peer, GossipLinkStoreMessageSubjects.LINK_ANTI_ENTROPY_ADVERTISEMENT, ad); | ||
| 548 | + } catch (Exception e) { | ||
| 549 | + log.error("Failed to send anti-entropy advertisement", e); | ||
| 550 | + return; | ||
| 551 | + } | ||
| 552 | + } catch (Exception e) { | ||
| 553 | + // catch all Exception to avoid Scheduled task being suppressed. | ||
| 554 | + log.error("Exception thrown while sending advertisement", e); | ||
| 555 | + } | ||
| 556 | + } | ||
| 557 | + } | ||
| 558 | + | ||
| 559 | + private LinkAntiEntropyAdvertisement createAdvertisement() { | ||
| 560 | + final NodeId self = clusterService.getLocalNode().id(); | ||
| 561 | + | ||
| 562 | + Map<LinkFragmentId, Timestamp> linkTimestamps = new HashMap<>(linkDescs.size()); | ||
| 563 | + Map<LinkKey, Timestamp> linkTombstones = new HashMap<>(removedLinks.size()); | ||
| 564 | + | ||
| 565 | + for (Entry<LinkKey, ConcurrentMap<ProviderId, Timestamped<LinkDescription>>> | ||
| 566 | + provs : linkDescs.entrySet()) { | ||
| 567 | + | ||
| 568 | + final LinkKey linkKey = provs.getKey(); | ||
| 569 | + final ConcurrentMap<ProviderId, Timestamped<LinkDescription>> linkDesc = provs.getValue(); | ||
| 570 | + synchronized (linkDesc) { | ||
| 571 | + for (Map.Entry<ProviderId, Timestamped<LinkDescription>> e : linkDesc.entrySet()) { | ||
| 572 | + linkTimestamps.put(new LinkFragmentId(linkKey, e.getKey()), e.getValue().timestamp()); | ||
| 573 | + } | ||
| 574 | + } | ||
| 575 | + } | ||
| 576 | + | ||
| 577 | + linkTombstones.putAll(removedLinks); | ||
| 578 | + | ||
| 579 | + return new LinkAntiEntropyAdvertisement(self, linkTimestamps, linkTombstones); | ||
| 580 | + } | ||
| 581 | + | ||
| 582 | + private void handleAntiEntropyAdvertisement(LinkAntiEntropyAdvertisement advertisement) { | ||
| 583 | + | ||
| 584 | + NodeId peer = advertisement.sender(); | ||
| 585 | + | ||
| 586 | + Map<LinkFragmentId, Timestamp> linkTimestamps = advertisement.linkTimestamps(); | ||
| 587 | + Map<LinkKey, Timestamp> linkTombstones = advertisement.linkTombstones(); | ||
| 588 | + for (Map.Entry<LinkFragmentId, Timestamp> entry : linkTimestamps.entrySet()) { | ||
| 589 | + LinkFragmentId linkFragmentId = entry.getKey(); | ||
| 590 | + Timestamp peerTimestamp = entry.getValue(); | ||
| 591 | + | ||
| 592 | + LinkKey key = linkFragmentId.linkKey(); | ||
| 593 | + ProviderId providerId = linkFragmentId.providerId(); | ||
| 594 | + | ||
| 595 | + Timestamped<LinkDescription> linkDescription = getLinkDescription(key, providerId); | ||
| 596 | + if (linkDescription.isNewer(peerTimestamp)) { | ||
| 597 | + // I have more recent link description. update peer. | ||
| 598 | + notifyPeer(peer, new InternalLinkEvent(providerId, linkDescription)); | ||
| 599 | + } | ||
| 600 | + // else TODO: Peer has more recent link description. request it. | ||
| 601 | + | ||
| 602 | + Timestamp linkRemovedTimestamp = removedLinks.get(key); | ||
| 603 | + if (linkRemovedTimestamp != null && linkRemovedTimestamp.compareTo(peerTimestamp) > 0) { | ||
| 604 | + // peer has a zombie link. update peer. | ||
| 605 | + notifyPeer(peer, new InternalLinkRemovedEvent(key, linkRemovedTimestamp)); | ||
| 606 | + } | ||
| 607 | + } | ||
| 608 | + | ||
| 609 | + for (Map.Entry<LinkKey, Timestamp> entry : linkTombstones.entrySet()) { | ||
| 610 | + LinkKey key = entry.getKey(); | ||
| 611 | + Timestamp peerTimestamp = entry.getValue(); | ||
| 612 | + | ||
| 613 | + ProviderId primaryProviderId = pickPrimaryProviderId(getLinkDescriptions(key)); | ||
| 614 | + if (primaryProviderId != null) { | ||
| 615 | + if (!getLinkDescription(key, primaryProviderId).isNewer(peerTimestamp)) { | ||
| 616 | + notifyDelegateIfNotNull(removeLinkInternal(key, peerTimestamp)); | ||
| 617 | + } | ||
| 618 | + } | ||
| 619 | + } | ||
| 620 | + } | ||
| 621 | + | ||
| 459 | private class InternalLinkEventListener implements ClusterMessageHandler { | 622 | private class InternalLinkEventListener implements ClusterMessageHandler { |
| 460 | @Override | 623 | @Override |
| 461 | public void handle(ClusterMessage message) { | 624 | public void handle(ClusterMessage message) { |
| ... | @@ -483,4 +646,14 @@ public class GossipLinkStore | ... | @@ -483,4 +646,14 @@ public class GossipLinkStore |
| 483 | notifyDelegateIfNotNull(removeLinkInternal(linkKey, timestamp)); | 646 | notifyDelegateIfNotNull(removeLinkInternal(linkKey, timestamp)); |
| 484 | } | 647 | } |
| 485 | } | 648 | } |
| 649 | + | ||
| 650 | + private final class InternalLinkAntiEntropyAdvertisementListener implements ClusterMessageHandler { | ||
| 651 | + | ||
| 652 | + @Override | ||
| 653 | + public void handle(ClusterMessage message) { | ||
| 654 | + log.info("Received Link Anti-Entropy advertisement from peer: {}", message.sender()); | ||
| 655 | + LinkAntiEntropyAdvertisement advertisement = SERIALIZER.decode(message.payload()); | ||
| 656 | + handleAntiEntropyAdvertisement(advertisement); | ||
| 657 | + } | ||
| 658 | + } | ||
| 486 | } | 659 | } | ... | ... |
| ... | @@ -9,6 +9,10 @@ public final class GossipLinkStoreMessageSubjects { | ... | @@ -9,6 +9,10 @@ public final class GossipLinkStoreMessageSubjects { |
| 9 | 9 | ||
| 10 | private GossipLinkStoreMessageSubjects() {} | 10 | private GossipLinkStoreMessageSubjects() {} |
| 11 | 11 | ||
| 12 | - public static final MessageSubject LINK_UPDATE = new MessageSubject("peer-link-update"); | 12 | + public static final MessageSubject LINK_UPDATE = |
| 13 | - public static final MessageSubject LINK_REMOVED = new MessageSubject("peer-link-removed"); | 13 | + new MessageSubject("peer-link-update"); |
| 14 | + public static final MessageSubject LINK_REMOVED = | ||
| 15 | + new MessageSubject("peer-link-removed"); | ||
| 16 | + public static final MessageSubject LINK_ANTI_ENTROPY_ADVERTISEMENT = | ||
| 17 | + new MessageSubject("link-enti-entropy-advertisement"); | ||
| 14 | } | 18 | } | ... | ... |
core/store/dist/src/main/java/org/onlab/onos/store/link/impl/LinkAntiEntropyAdvertisement.java
0 → 100644
| 1 | +package org.onlab.onos.store.link.impl; | ||
| 2 | + | ||
| 3 | +import static com.google.common.base.Preconditions.checkNotNull; | ||
| 4 | + | ||
| 5 | +import java.util.Map; | ||
| 6 | + | ||
| 7 | +import org.onlab.onos.cluster.NodeId; | ||
| 8 | +import org.onlab.onos.net.LinkKey; | ||
| 9 | +import org.onlab.onos.store.Timestamp; | ||
| 10 | + | ||
| 11 | +/** | ||
| 12 | + * Link AE Advertisement message. | ||
| 13 | + */ | ||
| 14 | +public class LinkAntiEntropyAdvertisement { | ||
| 15 | + | ||
| 16 | + private final NodeId sender; | ||
| 17 | + private final Map<LinkFragmentId, Timestamp> linkTimestamps; | ||
| 18 | + private final Map<LinkKey, Timestamp> linkTombstones; | ||
| 19 | + | ||
| 20 | + | ||
| 21 | + public LinkAntiEntropyAdvertisement(NodeId sender, | ||
| 22 | + Map<LinkFragmentId, Timestamp> linkTimestamps, | ||
| 23 | + Map<LinkKey, Timestamp> linkTombstones) { | ||
| 24 | + this.sender = checkNotNull(sender); | ||
| 25 | + this.linkTimestamps = checkNotNull(linkTimestamps); | ||
| 26 | + this.linkTombstones = checkNotNull(linkTombstones); | ||
| 27 | + } | ||
| 28 | + | ||
| 29 | + public NodeId sender() { | ||
| 30 | + return sender; | ||
| 31 | + } | ||
| 32 | + | ||
| 33 | + public Map<LinkFragmentId, Timestamp> linkTimestamps() { | ||
| 34 | + return linkTimestamps; | ||
| 35 | + } | ||
| 36 | + | ||
| 37 | + public Map<LinkKey, Timestamp> linkTombstones() { | ||
| 38 | + return linkTombstones; | ||
| 39 | + } | ||
| 40 | + | ||
| 41 | + // For serializer | ||
| 42 | + @SuppressWarnings("unused") | ||
| 43 | + private LinkAntiEntropyAdvertisement() { | ||
| 44 | + this.sender = null; | ||
| 45 | + this.linkTimestamps = null; | ||
| 46 | + this.linkTombstones = null; | ||
| 47 | + } | ||
| 48 | +} |
| 1 | +package org.onlab.onos.store.link.impl; | ||
| 2 | + | ||
| 3 | +import java.util.Objects; | ||
| 4 | + | ||
| 5 | +import org.onlab.onos.net.LinkKey; | ||
| 6 | +import org.onlab.onos.net.provider.ProviderId; | ||
| 7 | + | ||
| 8 | +import com.google.common.base.MoreObjects; | ||
| 9 | + | ||
| 10 | +/** | ||
| 11 | + * Identifier for LinkDescription from a Provider. | ||
| 12 | + */ | ||
| 13 | +public final class LinkFragmentId { | ||
| 14 | + public final ProviderId providerId; | ||
| 15 | + public final LinkKey linkKey; | ||
| 16 | + | ||
| 17 | + public LinkFragmentId(LinkKey linkKey, ProviderId providerId) { | ||
| 18 | + this.providerId = providerId; | ||
| 19 | + this.linkKey = linkKey; | ||
| 20 | + } | ||
| 21 | + | ||
| 22 | + public LinkKey linkKey() { | ||
| 23 | + return linkKey; | ||
| 24 | + } | ||
| 25 | + | ||
| 26 | + public ProviderId providerId() { | ||
| 27 | + return providerId; | ||
| 28 | + } | ||
| 29 | + | ||
| 30 | + @Override | ||
| 31 | + public int hashCode() { | ||
| 32 | + return Objects.hash(providerId, linkKey); | ||
| 33 | + } | ||
| 34 | + | ||
| 35 | + @Override | ||
| 36 | + public boolean equals(Object obj) { | ||
| 37 | + if (this == obj) { | ||
| 38 | + return true; | ||
| 39 | + } | ||
| 40 | + if (!(obj instanceof LinkFragmentId)) { | ||
| 41 | + return false; | ||
| 42 | + } | ||
| 43 | + LinkFragmentId that = (LinkFragmentId) obj; | ||
| 44 | + return Objects.equals(this.linkKey, that.linkKey) && | ||
| 45 | + Objects.equals(this.providerId, that.providerId); | ||
| 46 | + } | ||
| 47 | + | ||
| 48 | + @Override | ||
| 49 | + public String toString() { | ||
| 50 | + return MoreObjects.toStringHelper(getClass()) | ||
| 51 | + .add("providerId", providerId) | ||
| 52 | + .add("linkKey", linkKey) | ||
| 53 | + .toString(); | ||
| 54 | + } | ||
| 55 | + | ||
| 56 | + // for serializer | ||
| 57 | + @SuppressWarnings("unused") | ||
| 58 | + private LinkFragmentId() { | ||
| 59 | + this.providerId = null; | ||
| 60 | + this.linkKey = null; | ||
| 61 | + } | ||
| 62 | +} |
-
Please register or login to post a comment